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 4: clarify onion test payload contents #1077

Merged
merged 1 commit into from
May 10, 2023

Conversation

ellemouton
Copy link
Contributor

@ellemouton ellemouton commented May 7, 2023

This PR adds a clarifying comment to the onion-test.json file. The previous multi-frame test did not have lengths of payloads included in the hop payload field and instead had a separate field defining the payload type. This meant that the variable lengths still had to be added for the tlv payloads. However, in this new test - all the hops use the TLV type and so the payloads in this test do include the encoded lengths. This PR just adds a comment to clarify this.

@lightning lightning deleted a comment from Daddysgotthis May 9, 2023
@ellemouton
Copy link
Contributor Author

sorry if I am completely wrong about this - It is just that I could only get things matching if I explicitly set the type to TLV. And things matched the current test vector if I set the type to legacy. So thought maybe it incorrect. But maybe Im also doing something wrong on my side

@t-bast
Copy link
Collaborator

t-bast commented May 9, 2023

I think I understand what's happening. The difference between your onion and the spec one is that you are pre-pending each hop.payload with its length, but they already contain their length.

For example, the first hop payload in the test vector is 12 02023a98 040205dc 06080000000000000001 where 12 is the length of the following tlv stream. In your onion, the first hop payload is 13 12 02023a98 040205dc 06080000000000000001.

The test case comment could be updated to make this explicit, this is really hard to figure out.

@ellemouton
Copy link
Contributor Author

ah! ok that makes way more sense! Thanks for the help here @t-bast 🙏

I can update this PR so that it adds a clarifying comment to the test

Adds a comment to the `onion-test.json` file to clarify that the
payloads specified for each hop in the test already include the variable
length encodings.
@ellemouton ellemouton changed the title Bolt 4: fix onion test vector Bolt 4: clarify onion test payload contents May 10, 2023
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Thanks!

@t-bast t-bast added the clarification substantive change or addition around wording or meaning label May 10, 2023
@t-bast t-bast merged commit 1c1eda0 into lightning:master May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification substantive change or addition around wording or meaning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants