Provide payment preimages to signer on HTLC success #1251

devrandom · 2022-01-18T18:50:26Z

So that signer can validate that the local balance decrease is justified by a successful payment.

TheBlueMatt

Aside from one trivial MSRV violation, this basically LGTM - do you want to land this as-is or do you want to do the signer changes in the same pr (fine with me either way)

lightning/src/ln/channel.rs

codecov-commenter · 2022-01-19T11:42:47Z

Codecov Report

Merging #1251 (94a3db9) into main (35d4ebb) will decrease coverage by 0.00%.
The diff coverage is 83.87%.

❗ Current head 94a3db9 differs from pull request most recent head 6e19d1f. Consider uploading reports for the commit 6e19d1f to get more accurate results

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
- Coverage   90.38%   90.38%   -0.01%     
==========================================
  Files          71       70       -1     
  Lines       38135    38165      +30     
==========================================
+ Hits        34470    34495      +25     
- Misses       3665     3670       +5

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`89.17% <82.55%> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`95.75% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.32% <100.00%> (+0.04%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`89.47% <100.00%> (ø)`
lightning-background-processor/src/lib.rs	`92.98% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.28% <0.00%> (-0.02%)`	⬇️
lightning/src/util/test_utils.rs	`82.21% <0.00%> (ø)`
lightning/src/util/invoice.rs
... and 4 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 35d4ebb...6e19d1f. Read the comment docs.

devrandom · 2022-01-19T11:43:35Z

I added the signer changes, they were not complex

devrandom · 2022-01-19T13:40:50Z

Just to make sure - it looks to me that the lifetime of OutboundHTLCOutput objects is longer than the signature operations (sign_counterparty_commitment and validate_holder_commitment), and in particular, they won't be removed before both sides revoke the previous state.

TheBlueMatt · 2022-01-19T16:35:46Z

it looks to me that the lifetime of OutboundHTLCOutput objects is longer than the signature operations (sign_counterparty_commitment and validate_holder_commitment), and in particular, they won't be removed before both sides revoke the previous state.

That is correct.

TheBlueMatt

Basically LGTM, docs could be clearer, though.

TheBlueMatt · 2022-01-19T16:39:40Z

lightning/src/ln/channel.rs

+			let preimage_opt = match htlc.state {
+				OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) => p,
+				OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) => p,
+				OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p,


In this state, we've already sent a CS with the HTLCs removed, and are just waiting on a remote RAA to "forget" the HTLC. IIUC, because the HTLC will be removed before we sign the next remote CS, this won't lead to entries in sign_counterparty_commitment that we don't actually need (though we may need to re-sign the same CS again in case of reconnect), but it may mean we include the preimage in a validate_holder_commitment call even though, from the pov of the signer, its been fully and completely removed everywhere.

For a 1st pass, I think this is fine, but we should definitely document in the signer interface that we can include some preimages that were already fully removed and we prefer to over-provide preimages rather than under-provide them - its up to the signer to figure out what to do with them.

should we just say:

/// NOTE: a preimage may be provided here even if it has been already provided previously /// in [`sign_counterparty_commitment`].

in validate_holder_commitment?

Likely, yea, I would word it even more aggressively and just disclaim any guarantees that the preimages are at all relevant and aren't just random strings.

pushed doc changes

TheBlueMatt · 2022-01-20T17:07:14Z

lightning/src/ln/channel.rs

 				log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
 					log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id));
-				htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason);
+				htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason.clone().into());


Yuck. Can we mem::swap with a dummy value to avoid cloning Vecs just to drop? Same below.

TheBlueMatt

Not a fan of all the clones of Vecs, which I think we can avoid, but otherwise looks good.

TheBlueMatt · 2022-01-20T17:10:23Z

lightning/src/ln/channel.rs

+					if let OutboundHTLCOutcome::Success(preimage) = outcome {
+						preimages.push(preimage);
+					}
+					let reason: Option<HTLCFailReason> = outcome.clone().into();


Can we avoid this clone as well? Instead of cloneing and converting, we can add a method to write as an option with a reference inside the option?

fixed, and the one below too

TheBlueMatt

Otherwise LGTM, I believe.

TheBlueMatt · 2022-01-20T18:37:40Z

lightning/src/ln/channel.rs

+				// Grab the preimage, if it exists, instead of cloning
+				let mut reason = OutboundHTLCOutcome::Success(None);
+				mem::swap(fail_reason, &mut reason);
+				htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason.into());


nit: I believe the .into() here is unnecessary, and below.

valentinewallace · 2022-01-24T18:52:05Z

lightning/src/ln/channel.rs

-			if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {
-				Some(fail_reason.take())
-			} else { None } {
+			if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {


nit: would be nice to rename fail_reason to outcome here, similar below

done here and in a few other places

devrandom · 2022-01-24T20:50:50Z

Rebased. Sorry @TheBlueMatt - it dismissed your review even though there was no change.

TheBlueMatt requested changes Jan 18, 2022

View reviewed changes

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

devrandom force-pushed the 2022-01-signer-preimages branch from 732e196 to 6c4cf86 Compare January 19, 2022 11:42

devrandom marked this pull request as ready for review January 19, 2022 11:43

devrandom force-pushed the 2022-01-signer-preimages branch from 6c4cf86 to ade4454 Compare January 19, 2022 12:52

TheBlueMatt reviewed Jan 19, 2022

View reviewed changes

devrandom force-pushed the 2022-01-signer-preimages branch from ade4454 to 5ece12a Compare January 19, 2022 19:47

TheBlueMatt reviewed Jan 20, 2022

View reviewed changes

devrandom force-pushed the 2022-01-signer-preimages branch from 5ece12a to 30b408b Compare January 20, 2022 17:52

TheBlueMatt reviewed Jan 20, 2022

View reviewed changes

devrandom force-pushed the 2022-01-signer-preimages branch from 30b408b to 94a3db9 Compare January 20, 2022 22:55

TheBlueMatt previously approved these changes Jan 21, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Jan 22, 2022

valentinewallace self-requested a review January 24, 2022 20:10

valentinewallace reviewed Jan 24, 2022

View reviewed changes

devrandom dismissed TheBlueMatt’s stale review via 735a0fa January 24, 2022 20:50

devrandom force-pushed the 2022-01-signer-preimages branch from 94a3db9 to 735a0fa Compare January 24, 2022 20:50

devrandom added 2 commits January 24, 2022 21:53

Keep track of preimage in OutboundHTLCState on success

9aa786c

Provide preimages to signer

6e19d1f

devrandom force-pushed the 2022-01-signer-preimages branch from 735a0fa to 6e19d1f Compare January 24, 2022 20:53

valentinewallace approved these changes Jan 25, 2022

View reviewed changes

TheBlueMatt approved these changes Jan 25, 2022

View reviewed changes

TheBlueMatt merged commit 3baaebe into lightningdevkit:main Jan 25, 2022

Provide payment preimages to signer on HTLC success #1251

Provide payment preimages to signer on HTLC success #1251

Uh oh!

Conversation

devrandom commented Jan 18, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devrandom commented Jan 19, 2022

Uh oh!

devrandom commented Jan 19, 2022

Uh oh!

TheBlueMatt commented Jan 19, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom commented Jan 24, 2022

Uh oh!

Uh oh!

codecov-commenter commented Jan 19, 2022 •

edited

Loading