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

Update commitment test vectors #2045

Merged

Conversation

wpaulino
Copy link
Contributor

Updates the commitment vectors to match against lightning/bolts#1056 and the latest changes to lightning/bolts#1018.

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2023

Codecov Report

Patch coverage: 77.77% and project coverage change: -3.80 ⚠️

Comparison is base (7b2537b) 91.13% compared to head (8bd5bdf) 87.33%.

❗ Current head 8bd5bdf differs from pull request most recent head 5910802. Consider uploading reports for the commit 5910802 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2045      +/-   ##
==========================================
- Coverage   91.13%   87.33%   -3.80%     
==========================================
  Files         101      101              
  Lines       48852    44297    -4555     
  Branches    48852    44297    -4555     
==========================================
- Hits        44521    38687    -5834     
- Misses       4331     5610    +1279     
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 84.00% <77.77%> (-5.37%) ⬇️
lightning/src/chain/chaininterface.rs 41.17% <0.00%> (-54.07%) ⬇️
lightning/src/sync/nostd_sync.rs 42.50% <0.00%> (-50.69%) ⬇️
lightning/src/offers/payer.rs 50.00% <0.00%> (-50.00%) ⬇️
lightning/src/util/time.rs 40.00% <0.00%> (-45.19%) ⬇️
lightning-rapid-gossip-sync/src/error.rs 23.07% <0.00%> (-36.93%) ⬇️
lightning/src/util/byte_utils.rs 64.28% <0.00%> (-35.72%) ⬇️
lightning/src/offers/parse.rs 61.36% <0.00%> (-31.97%) ⬇️
lightning/src/util/persist.rs 73.33% <0.00%> (-26.67%) ⬇️
lightning/src/util/logger.rs 59.64% <0.00%> (-24.73%) ⬇️
... and 92 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

wpaulino added 2 commits March 6, 2023 10:59
Tests the case where only one anchor output exists for the funder in the
commitment transaction due to the remote having a dust balance (in this
case, 0).
The amount for HTLC lightningdevkit#6 was updated in the spec's test vectors, but the
"same amount and preimage" test vector itself was not updated, even
though the new HTLC amount resulted in a different commitment
transaction, and thus, different signatures.
@wpaulino wpaulino force-pushed the fix-broken-commitment-test-vectors branch from 8bd5bdf to 5910802 Compare March 6, 2023 19:00
@TheBlueMatt
Copy link
Collaborator

Searching the resulting channel.rs for the various hex strings in the BOLT PRs linked makes me think we're missing a lot of tests, is that deliberate? eg just looking at the top of 1056 I can't find 6987999999 anywhere, or the hex part of

    output commit_tx: 02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8005d007000000000000220020748eba944fedc8827f6b06bc44678f93c0f9e6078b35c6331ed31e75f8ce0c2d8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121b8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121bc0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110a69f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e0400483045022100d25455151be075bae8b3400d0825341a3c25a1a5258b84ad2546c09539a83bc602203c1a4ac19c3ac415af7f6a98348f8d7e94fc0ab82dbed54c3c40134f465d027f0147304402206cda85b2811a211aa70fb74abf23303d87c4355ccf2c2c7954d4137c4fb26a830220719402ab3fef1cbaf42ba42fe437e9bed1e45f84547d603bf7af3fb88f50193301475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220

@wpaulino
Copy link
Contributor Author

wpaulino commented Mar 8, 2023

That specific one is for legacy channels (pre-static-remote-key), which we don't support. Since we only support static_remote_key and (soon) anchors_zero_fee_htlc_tx, we only have those test vectors.

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Checked that the test vectors we have, matched.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks.

@TheBlueMatt TheBlueMatt merged commit 2d213a4 into lightningdevkit:main Mar 14, 2023
@wpaulino wpaulino deleted the fix-broken-commitment-test-vectors branch March 14, 2023 19:06
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.

4 participants