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

Clarify Bolt 3 htlc tx output test vectors #852

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Mar 5, 2021

It was sometimes unclear whether we indexed by the output or the htlc id.
This is a follow-up from discussions made in #539.

@t-bast
Copy link
Collaborator Author

t-bast commented Apr 7, 2021

@rustyrussell @jkczyz you may be interested in these test vector updates

@rustyrussell
Copy link
Collaborator

Tested-Ack: 2f0cce1

rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Apr 27, 2021
…sely

Still needs some massaging (we print HTLCs as we add them, rather then
in the final order, which requires a manual move in one test vector),
but this makes it more trivial to compare the output with the BOLT 3
text after lightning/bolts#852

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Apr 27, 2021
…sely

Still needs some massaging (we print HTLCs as we add them, rather then
in the final order, which requires a manual move in one test vector),
but this makes it more trivial to compare the output with the BOLT 3
text after lightning/bolts#852

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@t-bast
Copy link
Collaborator Author

t-bast commented Apr 27, 2021

@jkczyz can you test this for the rust-lightning side? If these test vectors work for you as well, we'll merge this.

@jkczyz
Copy link
Contributor

jkczyz commented Apr 29, 2021

@t-bast It's unclear to me what has changed in these test vectors. A cursory look seems to suggest only labels have changed. Am I missing something?

Also, I think the PR description and commit message aren't very clear.

It was sometimes unclear where we indexed by the output or the htlc id.

Which one is it then and what is being indexed?

@t-bast
Copy link
Collaborator Author

t-bast commented Apr 29, 2021

@jkczyz yes only the labels have changed, but you may (or may not) be using them in your tests.

It was previously unclear in some places whether the number that was in the comment for htlcs was the htlc_id or the index of the htlc output in the commit tx. We also clearly specify now which are htlc-success and which are htlc-timeout txs and have a more coherent grouping of signatures and txs.

It's mostly a cosmetic change, but it should make these test vectors easier to understand for newcomers (at least that's the goal!).

rustyrussell added a commit to ElementsProject/lightning that referenced this pull request May 3, 2021
…sely

Still needs some massaging (we print HTLCs as we add them, rather then
in the final order, which requires a manual move in one test vector),
but this makes it more trivial to compare the output with the BOLT 3
text after lightning/bolts#852

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 t-bast force-pushed the clarify-bolt3-test-vectors branch from 2f0cce1 to 2a01574 Compare May 27, 2021 06:54
@t-bast
Copy link
Collaborator Author

t-bast commented May 27, 2021

Rebased. @rustyrussell confirmed that c-lightning validates these test vectors, @jkczyz can you have a look at the latest state of the PR? I think it's worth merging, it makes the test vectors clearer for future readers.

@jkczyz
Copy link
Contributor

jkczyz commented May 27, 2021

ACK 2a01574

Sorry about the delay! RL's tests don't use any output indexes directly. Rather they use RL's state machine logic to build a commitment transaction from the included HTLCs. So the change itself shouldn't affect us.

Took a pass through and it seems easier to understand now. Though regardless of the change, the vectors could use better formatting, IMHO. Not sure if anyone is parsing these with code, but they don't seem to optimized for the reader. It's also not clear to me why local_htlc_signature lines start with # comments. But this outside the scope of the PR.

@t-bast
Copy link
Collaborator Author

t-bast commented May 27, 2021

Thanks @jkczyz, I agree that formatting to get something more easily machine-readable would be great.
We've talked about it for years, but the cost of change overran the benefits so no-one made a PR 🙂

@rustyrussell shall we merge this now that we have 3 implementations acking?

@t-bast t-bast merged commit 381650c into master Jun 21, 2021
@t-bast t-bast deleted the clarify-bolt3-test-vectors branch June 21, 2021 20:01
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.

3 participants