Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: receivable_msat to listpeers #3572

Merged
merged 7 commits into from
Mar 27, 2020

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Mar 5, 2020

This adds the receivable_msat amount to the lightning-cli listpeers output. The logic is similar to spendable_msat and applies for the maximum possible amount in one HTLC in that situation.

Adds code, test and manpage - Closes #3555

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5710421

two tiny nits, otherwise lgtm

lightningd/peer_control.c Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
doc/STYLE.md Outdated Show resolved Hide resolved
doc/STYLE.md Outdated Show resolved Hide resolved
lightningd/peer_control.c Outdated Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the feat/receivable_msat branch 4 times, most recently from 49b2350 to 7d1f841 Compare March 8, 2020 13:39
tests/test_pay.py Outdated Show resolved Hide resolved
@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Mar 8, 2020

From my side, I'm more or less okay with this PR, aside I don't like the way I duplicated code (i.e. commit_txfee_spend/recv) with all just changing the REMOTE / LOCAL or our_config / their_config ... But any approach to unify the blocks lead to a iffy mess.

Also I think there might be an off by one in current (master) implementation, see https://github.com/ElementsProject/lightning/pull/3572/files/7d1f8416e1af8bf9579914db46da6ce5b5cb394a#diff-e6fd9e4a383e2ab7819a6292c1edf70bR2357 for my remark in the testcase.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side, I'm more or less okay with this PR, aside I don't like the way I duplicated code (i.e. commit_txfee_spend/recv) with all just changing the REMOTE / LOCAL or our_config / their_config ... But any approach to unify the blocks lead to a iffy mess.

We might want to extract the LOCAL/REMOTE parameter into the function signature, and then internally use side and !side whenever either is used in the function. This is already what we do in a couple of places, and the enum is such that !LOCAL == REMOTE and vice-versa :-)

Might save us from future divergence of the two copies of the code.

Otherwise we can follow up with cleanups for the off-by-one once we have identified a cause (I'm confident it's not something introduced in this PR). Opened #3595 so it doesn't get lost.

lightningd/peer_control.c Outdated Show resolved Hide resolved
lightningd/peer_control.c Outdated Show resolved Hide resolved
lightningd/peer_control.c Outdated Show resolved Hide resolved
doc/lightning-listpeers.7.md Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
doc/STYLE.md Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
@m-schmoock
Copy link
Collaborator Author

We might want to extract the LOCAL/REMOTE parameter into the function signature, and then internally use side and !side whenever either is used in the function. This is already what we do in a couple of places, and the enum is such that !LOCAL == REMOTE and vice-versa :-)

I'll check if I can come up with something...

@m-schmoock m-schmoock force-pushed the feat/receivable_msat branch 2 times, most recently from c9d425a to 784ed72 Compare March 22, 2020 12:25
@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Mar 22, 2020

@cdecker Is there more I can do? I.e. squash
f9dde34 and 2b4f034 or merge the two spendable receivable blocks with all the similar duplicated comments line peer_control.c 725 following:

/* Compute how much we can send via this channel in one payment. */
if (!amount_msat_sub_sat(&spendable,
channel->our_msat,
channel->channel_info.their_config.channel_reserve))
spendable = AMOUNT_MSAT(0);
/* Take away any currently-offered HTLCs. */
subtract_offered_htlcs(channel, &spendable);
/* If we're funder, subtract txfees we'll need to spend this */
if (channel->funder == LOCAL) {
if (!amount_msat_sub_sat(&spendable, spendable,
commit_txfee(channel, spendable,
LOCAL)))
spendable = AMOUNT_MSAT(0);
}
/* We can't offer an HTLC less than the other side will accept. */
if (amount_msat_less(spendable,
channel->channel_info.their_config.htlc_minimum))
spendable = AMOUNT_MSAT(0);
/* We can't offer an HTLC over the max payment threshold either. */
if (amount_msat_greater(spendable, chainparams->max_payment))
spendable = chainparams->max_payment;
/* append spendable to JSON output */
json_add_amount_msat_compat(response, spendable,
"spendable_msatoshi", "spendable_msat");
/* Compute how much we can receive via this channel in one payment */
if (!amount_sat_sub_msat(&their_msat, channel->funding, channel->our_msat))
their_msat = AMOUNT_MSAT(0);
if (!amount_msat_sub_sat(&receivable,
their_msat,
channel->our_config.channel_reserve))
receivable = AMOUNT_MSAT(0);
/* Take away any currently-offered HTLCs. */
subtract_received_htlcs(channel, &receivable);
/* If they're funder, subtract txfees they'll need to spend this */
if (channel->funder == REMOTE) {
if (!amount_msat_sub_sat(&receivable, receivable,
commit_txfee(channel,
receivable, REMOTE)))
receivable = AMOUNT_MSAT(0);
}
/* They can't offer an HTLC less than what we will accept. */
if (amount_msat_less(receivable, channel->our_config.htlc_minimum))
receivable = AMOUNT_MSAT(0);
/* They can't offer an HTLC over the max payment threshold either. */
if (amount_msat_greater(receivable, chainparams->max_payment))
receivable = chainparams->max_payment;
/* append receivable to JSON output */
json_add_amount_msat_compat(response, receivable,
"receivable_msatoshi", "receivable_msat");

Suggestion

	/* Compute how much we can send/receive via this channel in one payment. */
	if (!amount_msat_sub_sat(&spendable, channel->our_msat,
				 channel->channel_info.their_config.channel_reserve))
		spendable = AMOUNT_MSAT(0);
	if (!amount_sat_sub_msat(&their_msat, channel->funding, channel->our_msat))
		their_msat = AMOUNT_MSAT(0);
	if (!amount_msat_sub_sat(&receivable, their_msat, channel->our_config.channel_reserve))
		receivable = AMOUNT_MSAT(0);

	/* Take away any currently-offered HTLCs. */
	subtract_offered_htlcs(channel, &spendable);
	subtract_received_htlcs(channel, &receivable);

	/* Substract the funders txfees */
	if (channel->funder == LOCAL) {
		if (!amount_msat_sub_sat(&spendable, spendable,
					 commit_txfee(channel, spendable, LOCAL)))
			spendable = AMOUNT_MSAT(0);
	} else {
		if (!amount_msat_sub_sat(&receivable, receivable,
					 commit_txfee(channel, receivable, REMOTE)))
			receivable = AMOUNT_MSAT(0);
	}

	/* Can't offer an HTLC less than the other side will accept. */
	if (amount_msat_less(spendable, channel->channel_info.their_config.htlc_minimum))
		spendable = AMOUNT_MSAT(0);
	if (amount_msat_less(receivable, channel->our_config.htlc_minimum))
		receivable = AMOUNT_MSAT(0);

	/* Can't offer an HTLC over the max payment threshold either. */
	if (amount_msat_greater(spendable, chainparams->max_payment))
		spendable = chainparams->max_payment;
	if (amount_msat_greater(receivable, chainparams->max_payment))
		receivable = chainparams->max_payment;

	/* append spendable/receivable to JSON output */
	json_add_amount_msat_compat(response, spendable,
				    "spendable_msatoshi", "spendable_msat");
	json_add_amount_msat_compat(response, receivable,
				    "receivable_msatoshi", "receivable_msat");

@cdecker
Copy link
Member

cdecker commented Mar 27, 2020

ACK 2b4f034

@cdecker cdecker merged commit 1aa8c90 into ElementsProject:master Mar 27, 2020
@m-schmoock m-schmoock deleted the feat/receivable_msat branch September 8, 2020 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: receivable_msat
3 participants