Clarify Bolt 3 htlc tx output test vectors #852
Conversation
|
@rustyrussell @jkczyz you may be interested in these test vector updates |
|
Tested-Ack: 2f0cce1 |
…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 <[email protected]>
…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 <[email protected]>
|
@jkczyz can you test this for the rust-lightning side? If these test vectors work for you as well, we'll merge this. |
|
@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.
Which one is it then and what is being indexed? |
|
@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 It's mostly a cosmetic change, but it should make these test vectors easier to understand for newcomers (at least that's the goal!). |
…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 <[email protected]>
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
force-pushed
the
clarify-bolt3-test-vectors
branch
from
2f0cce1 to
2a01574
Compare
|
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. |
|
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 |
|
Thanks @jkczyz, I agree that formatting to get something more easily machine-readable would be great. @rustyrussell shall we merge this now that we have 3 implementations acking? |
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).
It was sometimes unclear whether we indexed by the output or the htlc id.
This is a follow-up from discussions made in #539.