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

Use correct input sequence for HTLC claims from counterparty commitments #2606

Conversation

wpaulino
Copy link
Contributor

HTLC outputs, like the to_remote output, in commitment transactions with anchor outputs also have an additional 1 CSV constraint on the counterparty. When spending such outputs, their corresponding input needs to have their sequence set to 1. This was done for HTLC claims from holder commitments, but unfortunately not for counterparty commitments as we were lacking test coverage.

@wpaulino wpaulino added this to the 0.0.117 milestone Sep 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (1ac53ed) 88.84% compared to head (3c83783) 88.91%.
Report is 13 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
+ Coverage   88.84%   88.91%   +0.07%     
==========================================
  Files         113      113              
  Lines       84729    85938    +1209     
  Branches    84729    85938    +1209     
==========================================
+ Hits        75275    76412    +1137     
- Misses       7240     7282      +42     
- Partials     2214     2244      +30     
Files Coverage Δ
lightning/src/chain/package.rs 86.00% <90.47%> (-0.01%) ⬇️
lightning/src/ln/monitor_tests.rs 98.12% <86.95%> (-0.29%) ⬇️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HTLC outputs, like the `to_remote` output, in commitment transactions
with anchor outputs also have an additional `1 CSV` constraint on the
counterparty. When spending such outputs, their corresponding input
needs to have their sequence set to 1. This was done for HTLC claims
from holder commitments, but unfortunately not for counterparty
commitments as we were lacking test coverage.
@wpaulino wpaulino force-pushed the anchors-counterparty-htlc-claim-sequence branch from d3bc90d to 3c83783 Compare September 27, 2023 18:50
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Tested ACK 3c83783 by mutating new Sequence::from_consensus(1) tweaks in as_tx_input and see test_yield_anchors_events extensions fails accordingly.

See question if we’re missing test coverage or not on one path.

Sequence::ENABLE_RBF_NO_LOCKTIME
},
_ => {
debug_assert!(false, "This should not be reachable by 'untractable' or 'malleable with external funding' packages");
Copy link

Choose a reason for hiding this comment

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

Note to reviewer: in this context “malleable with external funding package” scope second-stage HTLC-timeout and HTLC-preimage claiming offered / received outputs on holder commitment transaction.

fn as_tx_input(&self, previous_output: BitcoinOutPoint) -> TxIn {
let sequence = match self {
PackageSolvingData::RevokedOutput(_) => Sequence::ENABLE_RBF_NO_LOCKTIME,
PackageSolvingData::RevokedHTLCOutput(_) => Sequence::ENABLE_RBF_NO_LOCKTIME,
Copy link

Choose a reason for hiding this comment

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

I think we’re testing the “anchor channel”-enabled revoked counterparty commitment offered HTLC outputs case in test_anchors_aggregated_revoked_htlc_tx though I’m unsure we’re testing the claim with justice transaction on revoked counterparty commitment received HTLC ? All routed payments L2029 are in the same direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's covered now in #2610.

@TheBlueMatt TheBlueMatt merged commit 336d815 into lightningdevkit:main Sep 28, 2023
15 checks passed
@wpaulino wpaulino deleted the anchors-counterparty-htlc-claim-sequence branch September 28, 2023 16:37
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.

5 participants