Add a `PaymentForwarded` Event #1004

TheBlueMatt · 2021-07-16T02:32:01Z

Based on #977.

It is useful for accounting and informational reasons for users to
be informed when a payment has been successfully forwarded. Thus,
when an HTLC which represents a forwarded leg is claimed, we
generate a new PaymentForwarded event.

This requires some additional plumbing to return HTLC values from
OnchainEvents. Further, when we have to go on-chain to claim the
inbound side of the payment, we do not inform the user of the fee
reward, as we cannot calculate it until we see what is confirmed
on-chain.

codecov · 2021-07-16T02:49:18Z

Codecov Report

Merging #1004 (b9dab4e) into main (1bb9e64) will decrease coverage by 0.02%.
The diff coverage is 70.86%.

❗ Current head b9dab4e differs from pull request most recent head 50f47ec. Consider uploading reports for the commit 50f47ec to get more accurate results

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
- Coverage   90.81%   90.79%   -0.03%     
==========================================
  Files          60       61       +1     
  Lines       31428    31578     +150     
==========================================
+ Hits        28542    28671     +129     
- Misses       2886     2907      +21

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.18% <ø> (+0.26%)`	⬆️
lightning/src/util/events.rs	`13.93% <0.00%> (-1.20%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.57% <60.00%> (-0.23%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.69% <82.22%> (+0.12%)`	⬆️
lightning/src/ln/channel.rs	`89.23% <83.33%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.27% <95.00%> (-0.12%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.74% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`99.70% <100.00%> (+0.07%)`	⬆️
lightning/src/ln/peer_handler.rs	`45.98% <0.00%> (-0.47%)`	⬇️
lightning-invoice/src/de.rs	`80.73% <0.00%> (-0.34%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bb9e64...50f47ec. Read the comment docs.

ariard · 2021-07-25T23:22:19Z

CHANGELOG.md

+   clients cannot read. The likelihood of this can be reduced by ensuring you
+   process all pending events immediately before serialization.
+
+


Mentions e00bf8d serialization relaxation? Maybe we'll have users eager to experiment with the odd type semantic

I don't believe you can currently (reasonably) serialize events in a way that LDK cares about - I believe we only (really) read them as a part of a ChannelManager or ChannelMonitor, so in order for users to get anything out of it they'd need to be editing ChannelManagers in the middle of the serialization logic.

lightning/src/util/events.rs

ariard · 2021-07-25T23:51:06Z

lightning/src/util/events.rs

+		///
+		/// If the channel which sent us the payment has been force-closed, we will claim the funds
+		/// via an on-chain transaction. In that case we do not consider the on-chain transaction
+		/// fees involved in such a claim and this value will be `None`.


Wait I think a onchain_value_satoshis value is passed in process_pending_monitor_events and consumed as part of the generated PaymentForwarded there ? Or are you thinking about another case ?

Wait, I'm confused, those are only about claims that our counterparty did on-chain, causing us to update off-chain state. Did I miss something about how HTLCUpdate events are generated?

I think Antoine's right, seems like this will always be non-None with the current code?

If claim_funds_from_hop hits the short_to_id.get(..) == None branch right at the top, it'll return (None, Err(None)) which gets translated into our ChannelMonitorUpdate-generation branch, but should still consider claimed_htlc to be true, resulting in claimed_value_msat being None.

You say this value can be None in the case where the backward channel has been force-closed and we won't fetch the claimed amount from onchain, thus only yielding None as earned fees. So IIUC, I think it's confusing to say "on-chain transaction fees", it should say "amount" or values" ?

See updated doc commit, is that sufficient?

valentinewallace · 2021-07-28T17:26:51Z

Rebase plz

The wait_threshold_conf!() macro in check_spend_holder_transaction was only used once, making it a good candidate for inlining at the callsite. Further, it incorrectly always logged that we were failing HTLCs from the "latest" commitment transaction, when it is sometimes actually failing HTLCs from the previous commitment transaction.

This should provide some additional future extensibility, allowing for new informational events which can be safely ignored to be ignored by older versions.

test_onchain_to_onchain_claim was connecting additional blocks in order to reach HTLC timeout and broadcast an HTLC-Timeout transaction, resulting in it not testing whether HTLC preimages are learned instantly in response to HTLC-Success transactions.

TheBlueMatt · 2021-07-28T17:50:07Z

Rebased.

CHANGELOG.md

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-07-29T17:39:35Z

lightning/src/ln/channelmanager.rs

+				if claimed_htlc {
+					if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
+						let fee_earned_msat = if let Some(claimed_htlc_value) = claimed_value_msat {
+							Some(claimed_htlc_value - forwarded_htlc_value)


No risk/need for mitigation of overflow here, right?

I think not, my logic is this:

for only-off-chain forwards + claims, the fee_insufficient check in decode_update_add_htlc_onion should prevent forwarded_htlc_value from ever being below claimed_htlc_value (obviously if it ever did happen that would be a massive bug anyway),

for on-chain claims leading to a backwards off-chain claim, claimed_htlc_value will always be the original received value, whereas forwarded_htlc_value can only decrease (as its rounded down to the nearest whole satoshi value), so the above applies.

we do not get here for off-chain claims leading to a backwards on-chain claim, which would be possible to underflow on.

valentinewallace · 2021-07-29T19:42:31Z

lightning/src/util/events.rs

+		fee_earned_msat: Option<u64>,
+		/// If this is `true`, the forwarded HTLC was claimed on-chain after a force-closure.
+		claim_from_onchain_tx: bool,


Could these be combined into an enum, something like:
ForwardClaim::OnChain
ForwardClaim::OffChain(fee_earned_msat: u64)

They are two separate concepts - one is about the forward edge of the HTLC, the other is about the backwards edge of the HTLC. Any suggestions for how to make the docs more clear?

valentinewallace · 2021-07-29T19:48:52Z

lightning/src/util/events.rs

+		///
+		/// If the channel which sent us the payment has been force-closed, we will claim the funds
+		/// via an on-chain transaction. In that case we do not consider the on-chain transaction
+		/// fees involved in such a claim and this value will be `None`.


I think Antoine's right, seems like this will always be non-None with the current code?

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-07-30T20:47:31Z

lightning/src/ln/channelmanager.rs

@@ -207,6 +207,12 @@ pub(super) enum HTLCFailReason {
 	}
 }

+/// Return value for claim_funds_from_hop
+struct ClaimFundsRes {


Could we switch this to be an enum, something along these lines:

enum ClaimFundsRes { PrevHopForceClosed, MonitorUpdateFailed((u64, PublicKey, MsgHandleErrInternal)), ... other states }

This refactor is @jkczyz -approved 😛

Hmm, I'm not sure that that's that much clearer. All four possible states of (None, Ok(..)), (Some(..), Ok(..)), (None, Err(..)), and (Some(..), Err(..)) are used, so we'd need at least four variants, and we'd have a really hard time keeping the callsite match DRY.

IMO seeing a descriptive name like PrevHopForceClosed makes the code more readable at a glance than seeing (None, Ok(..)).

It seems like the callsite might only need to repeat a few lines? Is that off?

I mean I can split it into a struct containing two enums, but that seems to border on enough volume that it becomes unreadable just by weight. Sticking it all in one enum (with four variants) would be really harsh for claim-funds, too, which only actually cares about one of the two fields, now it would have to have a match and conversion instead of being able to just access the field. I agree having names for things is useful, but I don't think I agree it is if it results in more code by volume including matches that the reader has to figure out. Maybe there's a way to rename the fields in ClaimFundsRes to make it more clear instead?

Briefly jammed with @jkczyz on this patch: valentinewallace@f9d338d It seems more readable and the code volume doesn't seem significantly increased!

Yea, I guess I underestimated the amount of parameter conversion in the current code. I do think the MonitorUpdateFail overload that can mean both "channel is bogus, closing" and Success, we claimed is a bit confusing, but its not strictly worse than it was. I took that commit and added one tweak.

valentinewallace

build is sad

Pretty much good with this PR!

lightning/src/util/events.rs

valentinewallace · 2021-08-03T15:06:09Z

lightning/src/util/events.rs

+		/// fees involved in such a claim and this value will be `None`. It is possible duplicate
+		/// `PaymentForwarded` events are generated for the same payment iff `fee_earned_msat` is
+		/// `None`.


Because duplicates may be generated, could a (future?) direction be to add some kind of unique identifier of the HTLC to this event?

Yea, what we'd really need to do is have the event be generated by the ChannelMonitor if the inbound leg of the payment has gone on-chain, but there's a lot of complexity to ensuring we de-duplicate them all properly that we don't currently do that I figured it wasn't worth it for now.

Yeah we could return either a monotonic payment_id or PaymentHash at least.

or PaymentHash at least.

I'm a little torn on this. I initially didn't include it on purpose because, at least my thinking was, we really dont want users to log that or even care about it - its not their payment, and logging/giving users details about the payments others made (at least the parts that don't matter to you) is generally a bad idea.

On the other hand, I see the point about wanting to de-dup by them, but of course note that you can have the same PaymentHash multiple times, even if its insecure.

Ultimately I think adding PaymentHash is just not more confusing than helpful, especially since users may try to de-duplicate by it and have bad accounting, much better to just say you will have bad accounting, and leave it at that.

Right, we already have case where accounting might not be exact like rouding down or backward channel closed and where fee_earned_msat is None. We can revisit in the future if needed.

lightning/src/util/events.rs

lightning/src/ln/channelmanager.rs

valentinewallace

SGTM once build is fixed

lightning/src/util/events.rs

TheBlueMatt · 2021-08-03T17:13:07Z

once build is fixed

lol it was working locally. Anyway, I just dropped the whole partialeq thing. Also went ahead and squashed.

ariard · 2021-08-03T22:35:14Z

lightning/src/util/events.rs

+		/// fees involved in such a claim and this value will be `None`. It is possible duplicate
+		/// `PaymentForwarded` events are generated for the same payment iff `fee_earned_msat` is
+		/// `None`.


Yeah we could return either a monotonic payment_id or PaymentHash at least.

ariard · 2021-08-03T22:39:37Z

lightning/src/util/events.rs

+		/// `None`.
+		fee_earned_msat: Option<u64>,
+		/// If this is `true`, the forwarded HTLC will be claimed on-chain after the channel on
+		/// which we received the HTLC was force-closed.


I think claim_funds_internal() in process_pending_monitor_events is called with from_onchain=true. Though this callsite at monitor events processing doesn't imply the backward channel has been force-closed, a check we only realize at the beginning of claim_funds_from_hop.

So I think this comment is confusing...Shouldn't this say "we offered the HTLC was force-closed" to be correct ?

Lol, these docs are just entirely wrong. I pushed a fixup commit.

TheBlueMatt · 2021-08-04T16:45:15Z

Pushed an additional doc change commit to take changes from @ariard.

ariard · 2021-08-04T21:31:54Z

Code Review ACK 90c5d00

It is useful for accounting and informational reasons for users to be informed when a payment has been successfully forwarded. Thus, when an HTLC which represents a forwarded leg is claimed, we generate a new `PaymentForwarded` event. This requires some additional plumbing to return HTLC values from `OnchainEvent`s. Further, when we have to go on-chain to claim the inbound side of the payment, we do not inform the user of the fee reward, as we cannot calculate it until we see what is confirmed on-chain. Substantial code structure rewrites by: Valentine Wallace <[email protected]>

While we should never reach `ClaimFundsFromHop::DuplicateClaim` in most cases, if we do, it likely indicates the HTLC was timed out some time ago and is no longer available to be claimed. Thus, it does not make sense to imply that we `claimed_any_htlcs`.

TheBlueMatt · 2021-08-04T21:49:09Z

Squashed without changes. Diff since Val's review is all docs, so will merge after CI.

$ git diff-tree -U1 eebb0d1 50f47ecc
diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
index ae674c63..34b0b331 100644
--- a/lightning/src/util/events.rs
+++ b/lightning/src/util/events.rs
@@ -158,8 +158,10 @@ pub enum Event {
 		/// Note that if we force-closed the channel over which we forwarded an HTLC while the HTLC
-		/// was pending, the amount the next hop claimed may have been rounded down, causing a
-		/// higher fee than expected.
+		/// was pending, the amount the next hop claimed will have been rounded down to the nearest
+		/// whole satoshi. Thus, the fee calculated here may be higher than expected as we still
+		/// claimed the full value in millisatoshis from the source. In this case,
+		/// `claim_from_onchain_tx` will be set.
 		///
 		/// If the channel which sent us the payment has been force-closed, we will claim the funds
-		/// via an on-chain transaction. In that case we do not consider the on-chain transaction
-		/// fees involved in such a claim and this value will be `None`. It is possible duplicate
+		/// via an on-chain transaction. In that case we do not yet know the on-chain transaction
+		/// fees which we will spend and will instead set this to `None`. It is possible duplicate
 		/// `PaymentForwarded` events are generated for the same payment iff `fee_earned_msat` is
@@ -167,4 +169,4 @@ pub enum Event {
 		fee_earned_msat: Option<u64>,
-		/// If this is `true`, the forwarded HTLC will be claimed on-chain after the channel on
-		/// which we received the HTLC was force-closed.
+		/// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
+		/// transaction.
 		claim_from_onchain_tx: bool,
$

TheBlueMatt force-pushed the 2021-07-forward-event branch 2 times, most recently from 7cd334a to 25379af Compare July 16, 2021 03:17

TheBlueMatt mentioned this pull request Jul 22, 2021

Ensure proper accounting coverage of on-chain fees #1014

Open

ariard reviewed Jul 25, 2021

View reviewed changes

valentinewallace self-requested a review July 28, 2021 17:27

TheBlueMatt added 3 commits July 28, 2021 17:35

Ignore unknown Events serialized with an odd type value.

8ffc2d1

This should provide some additional future extensibility, allowing for new informational events which can be safely ignored to be ignored by older versions.

TheBlueMatt force-pushed the 2021-07-forward-event branch from 25379af to 997899d Compare July 28, 2021 17:49

valentinewallace reviewed Jul 28, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

valentinewallace reviewed Jul 29, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Jul 30, 2021

View reviewed changes

valentinewallace reviewed Aug 3, 2021

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-07-forward-event branch from 9a6398d to eebb0d1 Compare August 3, 2021 17:12

valentinewallace approved these changes Aug 3, 2021

View reviewed changes

ariard reviewed Aug 3, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-forward-event branch 2 times, most recently from b9dab4e to 90c5d00 Compare August 4, 2021 16:44

TheBlueMatt added 2 commits August 4, 2021 21:48

TheBlueMatt force-pushed the 2021-07-forward-event branch from 90c5d00 to 50f47ec Compare August 4, 2021 21:48

TheBlueMatt merged commit 69ee486 into lightningdevkit:main Aug 4, 2021

		clients cannot read. The likelihood of this can be reduced by ensuring you
		process all pending events immediately before serialization.

Add a PaymentForwarded Event #1004

Add a PaymentForwarded Event #1004

Conversation

TheBlueMatt commented Jul 16, 2021

codecov bot commented Jul 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jul 28, 2021

TheBlueMatt commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 4, 2021

ariard commented Aug 4, 2021

TheBlueMatt commented Aug 4, 2021

Add a `PaymentForwarded` Event #1004

Add a `PaymentForwarded` Event #1004

codecov bot commented Jul 16, 2021 •

edited

Loading

ariard Aug 3, 2021 •

edited

Loading

TheBlueMatt Jul 30, 2021 •

edited

Loading

TheBlueMatt Aug 4, 2021 •

edited

Loading