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

BOLT 3: add test vectors for htlc-transactions in case where CLTV is used as tie-breaker for sorting #539

Merged

Conversation

araspitzu
Copy link
Contributor

@araspitzu araspitzu commented Jan 7, 2019

Add a serialized transactions test vector for the edge case of sorting htlc-timeout-tx when there are multiple offered htlc with the same amount and preimage. The test vector reuses previous preimages and creates a case scenario with 1 received htlc and 2 offered, the two offered will have same scriptPubKey and redeemScript, but different CLTV value. It is asserted the order in which the htlc transactions should be kept internally and we assume the same order is used to construct the commitment_signed message. This complements #491 .

@araspitzu araspitzu changed the title Add test vectors for htlc-transactions for the case when the CLTV is used as tie-breaker for sorting Add test vectors for htlc-transactions for the case when CLTV is used as tie-breaker for sorting Jan 7, 2019
@araspitzu araspitzu changed the title Add test vectors for htlc-transactions for the case when CLTV is used as tie-breaker for sorting BOLT 3: add test vectors for htlc-transactions in case where CLTV is used as tie-breaker for sorting Jan 7, 2019
03-transactions.md Outdated Show resolved Hide resolved
@ysangkok
Copy link
Contributor

This seems like it would still be useful. @rustyrussell , the meeting logs say you validated these vectors. What was the result?

@t-bast t-bast force-pushed the ordering_CLTV_tiebreaker_test_vectors branch from 8e8b4da to 1482e1f Compare December 23, 2020 14:48
@t-bast
Copy link
Collaborator

t-bast commented Dec 23, 2020

As discussed during the spec meeting, I added the corresponding anchor output test vector.
This is waiting for a second implementation to validate the test vectors.

03-transactions.md Outdated Show resolved Hide resolved
03-transactions.md Outdated Show resolved Hide resolved
t-bast added a commit to ACINQ/eclair that referenced this pull request Jan 8, 2021
To match the latest test vectors in lightning/bolts#539
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

@t-bast It occurred to me while testing this that adding the HTLCs in the order stated (i.e., the expected ordering) won't test the intended behavior since the HTLCs would be presorted. Case in point, shouldn't the outgoing HTLC ordering be switched in Eclair's test?

Perhaps a comment in the BOLT on the new test vector would help convey this.

03-transactions.md Outdated Show resolved Hide resolved
Comment on lines 1446 to 1454
# local_htlc_signature = 304402203198b389ca301ee713eef7beb2b73655fa28eb25ac3cb2e6b295767776fe67a402206e7dcfbcd5ce2bcadf254ba9911c9a770d3e1b752b276575ba8b9fd1c2b424e7
# remote_htlc_signature = 3044022031694c29682ed2646dc6b64f59236f7d262027cac742b89030692483ca8326fe022040ca3216802d232eb04894f81008785b82b74812983fc05f93db7db5ff5ef613
output htlc_success_tx 0: 0200000000010175a53c9a465f56140f1c7b2c42b27f0e56b508e39cb6971f3c5b551cfda8e5e4000000000000000000011f070000000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e0500473044022031694c29682ed2646dc6b64f59236f7d262027cac742b89030692483ca8326fe022040ca3216802d232eb04894f81008785b82b74812983fc05f93db7db5ff5ef6130147304402203198b389ca301ee713eef7beb2b73655fa28eb25ac3cb2e6b295767776fe67a402206e7dcfbcd5ce2bcadf254ba9911c9a770d3e1b752b276575ba8b9fd1c2b424e7012001010101010101010101010101010101010101010101010101010101010101018a76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c8201208763a9144b6b2e5444c2639cc0fb7bcea5afba3f3cdce23988527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae677502f501b175ac686800000000
# local_htlc_signature = 304402201ef4ba8337e517980256d61301ab83e528d4aab79c9610066ef200a6b252d58d0220277cebbbff3ac7d1fecafd8befe1525470b3ee180d282fc8c36e0237c96d9739
# remote_htlc_signature = 3045022100b3884c0174eb3650e68807bba145a752f8769b4ec886e95553f78380edbd28e6022058621ac2f742239f7be25b549d2f3bad9a1524f2b346a247fcb151afb1c4c5eb
output htlc_timeout_tx 1: 0200000000010175a53c9a465f56140f1c7b2c42b27f0e56b508e39cb6971f3c5b551cfda8e5e401000000000000000001e1120000000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e0500483045022100b3884c0174eb3650e68807bba145a752f8769b4ec886e95553f78380edbd28e6022058621ac2f742239f7be25b549d2f3bad9a1524f2b346a247fcb151afb1c4c5eb0147304402201ef4ba8337e517980256d61301ab83e528d4aab79c9610066ef200a6b252d58d0220277cebbbff3ac7d1fecafd8befe1525470b3ee180d282fc8c36e0237c96d973901008576a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868f9010000
# local_htlc_signature = 30440220207ee7d74a0f223b5d05db49653c541d4a043f2ab3e88abbeeb5f1d1d761535902207de25db7f70a939eae6cec89d8487ad1e9c953cb0f30a5f84e5ade5736d6f9ce
# remote_htlc_signature = 3045022100de2757977d17699d092c1b05cebb52fa498de5d5fba6a60762f73aa7354ad8a50220199295de83336966e7387e8ebb2f105adc0db516260d6cd1e3c166f8b32d853e
output htlc_timeout_tx 2: 0200000000010175a53c9a465f56140f1c7b2c42b27f0e56b508e39cb6971f3c5b551cfda8e5e402000000000000000001e1120000000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e0500483045022100de2757977d17699d092c1b05cebb52fa498de5d5fba6a60762f73aa7354ad8a50220199295de83336966e7387e8ebb2f105adc0db516260d6cd1e3c166f8b32d853e014730440220207ee7d74a0f223b5d05db49653c541d4a043f2ab3e88abbeeb5f1d1d761535902207de25db7f70a939eae6cec89d8487ad1e9c953cb0f30a5f84e5ade5736d6f9ce01008576a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868fa010000
Copy link

Choose a reason for hiding this comment

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

Not sure which is a preferable ordering, but for consistency with the other test vectors could you list all the remote HTLC signatures together? Likewise for the option_static_remotekey variant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, these test vectors weren't completely aligned with the format of other test vectors, I think they were partially hand-generated instead of relying on our tests, I fixed that in 280e960

Case in point, shouldn't the outgoing HTLC ordering be switched in Eclair's test?

I see what you mean, done in ACINQ/eclair#1669
I think it's up to implementations to ensure this is properly tested.

jkczyz added a commit to jkczyz/rust-lightning that referenced this pull request Jan 8, 2021
jkczyz added a commit to jkczyz/rust-lightning that referenced this pull request Jan 8, 2021
jkczyz added a commit to jkczyz/rust-lightning that referenced this pull request Jan 8, 2021
Copy link
Collaborator

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

The option_static_remotekey HTLCs seem all wrong?

Comment on lines +1435 to +1438
name: commitment tx with 3 htlc outputs, 2 offered having the same amount and preimage
to_local_msat: 6988000000
to_remote_msat: 3000000000
local_feerate_per_kw: 253
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ack this test vector for 280e960.

I have added this to c-lightning unit tests, and confirmed exact match on the signatures and all transactions.

# to_remote amount 3000000 P2WPKH(032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991)
remote_signature = 3044022009b048187705a8cbc9ad73adbe5af148c3d012e1f067961486c822c7af08158c022006d66f3704cfab3eb2dc49dae24e4aa22a6910fc9b424007583204e3621af2e5
# local_signature = 304402206fc2d1f10ea59951eefac0b4b7c396a3c3d87b71ff0b019796ef4535beaf36f902201765b0181e514d04f4c8ad75659d7037be26cdb3f8bb6f78fe61decef484c3ea
output commit_tx: 02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8007e80300000000000022002052bfef0479d7b293c27e0f1eb294bea154c63a3294ef092c19af51409bce0e2ad007000000000000220020403d394747cae42e98ff01734ad5c08f82ba123d3d9a620abda88989651e2ab5d007000000000000220020748eba944fedc8827f6b06bc44678f93c0f9e6078b35c6331ed31e75f8ce0c2db80b000000000000220020c20b5d1f8584fd90443e7b7b720136174fa4b9333c261d04dbbd012635c0f419a00f0000000000002200208c48d15160397c9731df9bc3b236656efb6665fbfe92b4a6878e88a499f741c4c0c62d0000000000160014cc1b07838e387deacd0e5232e1e8b49f4c29e484e0a06a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040047304402206fc2d1f10ea59951eefac0b4b7c396a3c3d87b71ff0b019796ef4535beaf36f902201765b0181e514d04f4c8ad75659d7037be26cdb3f8bb6f78fe61decef484c3ea01473044022009b048187705a8cbc9ad73adbe5af148c3d012e1f067961486c822c7af08158c022006d66f3704cfab3eb2dc49dae24e4aa22a6910fc9b424007583204e3621af2e501475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is correct; the txid is 2b887d4c1c59cd605144a1e2f971d168437db453f841f2fefb2c164f28ff84ab.

03-transactions.md Outdated Show resolved Hide resolved
@t-bast
Copy link
Collaborator

t-bast commented Feb 17, 2021

The option_static_remotekey HTLCs seem all wrong?

That's worrying 😅
I'll investigate based on your comments.

t-bast added a commit to ACINQ/eclair that referenced this pull request Feb 17, 2021
To match the latest test vectors in lightning/bolts#539
@jkczyz
Copy link

jkczyz commented Mar 1, 2021

The option_static_remotekey HTLCs seem all wrong?

That's worrying 😅
I'll investigate based on your comments.

FYI, I confirmed that RL's test vectors do indeed match the corrected ones in c6d13fb.

@t-bast
Copy link
Collaborator

t-bast commented Mar 1, 2021

I have no idea how I messed that up, this one's completely on me!

shame

@rustyrussell
Copy link
Collaborator

Vectors now check out!

Ack c6d13fb

There is some weirdness in the formatting, which should be cleaned up in a separate PR.

  1. Some places use local_signature where it means local_htlc_signature.
  2. output htlc_success_tx 1 isn't clear whether this is output 1, or htlc id 1. Both are used in different places, and we should probably say output #1, htlc_success_tx id 1 etc.

@t-bast
Copy link
Collaborator

t-bast commented Mar 5, 2021

Great, thanks for checking!

As discussed during a previous spec meeting (#838), merging this PR now that it has been verified by both rust-lightning and c-lightning.

There is some weirdness in the formatting, which should be cleaned up in a separate PR.

I'll create a follow-up PR to apply these clarifications, that will indeed be useful for newcomers.

@t-bast t-bast merged commit b201efe into lightning:master Mar 5, 2021
t-bast added a commit that referenced this pull request Mar 5, 2021
It was sometimes unclear where we indexed by the output or the htlc id.

This is a follow-up from discussions made in #539.
t-bast added a commit to ACINQ/eclair that referenced this pull request Mar 5, 2021
To match the latest test vectors in lightning/bolts#539
t-bast added a commit that referenced this pull request May 27, 2021
It was sometimes unclear where we indexed by the output or the htlc id.

This is a follow-up from discussions made in #539.
t-bast added a commit that referenced this pull request Jun 21, 2021
It was sometimes unclear where we indexed by the output or the htlc id.
This is a follow-up from discussions made in #539.
t-bast added a commit to ACINQ/eclair that referenced this pull request Jun 22, 2021
To match the latest test vectors in lightning/bolts#539
t-bast added a commit to ACINQ/eclair that referenced this pull request Jun 22, 2021
Update bolt 3 spec test vectors to match the latest test vectors
from lightning/bolts#539 and
lightning/bolts#852 and
clarify HTLC outputs (see lightning/bolts#852).
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.

None yet

6 participants