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 #2: order htlc_signatures by BIP69 + increasing CLTV. #491

Merged
merged 2 commits into from Jan 22, 2019

Conversation

@rustyrussell
Copy link
Collaborator

@rustyrussell rustyrussell commented Oct 21, 2018

We express it has how the outputs are ordered, but the only way you can
detect that is by the htlc_signatures order, which is the part which really
matters.

I finally reproduced this, BTW, which is why I'm digging it up!

Closes: #448
Signed-off-by: Rusty Russell rusty@rustcorp.com.au

@rustyrussell rustyrussell requested a review from pm47 Oct 21, 2018
pm47
pm47 approved these changes Oct 21, 2018
Copy link
Collaborator

@pm47 pm47 left a comment

This is the simplest way to fix #448 I think.

I have just one bikeshed comment, mainly to try out the new suggestion github feature.

Loading

02-peer-protocol.md Outdated Show resolved Hide resolved
Loading
@TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented Oct 29, 2018

Can we just throw out the reference to BIP 69 and write out the full context there? (#456). This is really actually just a bug in BIP 69 as it should specify everything you need, but its a very poorly-written BIP.

Loading

@rustyrussell rustyrussell added this to the v1.0 milestone Nov 26, 2018
@rustyrussell
Copy link
Collaborator Author

@rustyrussell rustyrussell commented Nov 29, 2018

Implemented in c-lightning. Waiting on second implementation by LND or Eclair...

Loading

@araspitzu
Copy link
Contributor

@araspitzu araspitzu commented Dec 27, 2018

What about adding test vectors for the resulting htlc transactions? Although we should test against the htlc_signatures (in commitment_signed) having a test vector that specify the order of the htcl-tx associated with the commit-tx would be helpful for the implementers.

Loading

@rustyrussell
Copy link
Collaborator Author

@rustyrussell rustyrussell commented Jan 7, 2019

We should definitely have more test vectors, though this should be applied meanwhile IMHO.

Loading

rustyrussell and others added 2 commits Jan 22, 2019
We express it has how the outputs are ordered, but the only way you can
detect that is by the htlc_signatures order, which is the part which really
matters.

I finally reproduced this, BTW, which is why I'm digging it up!

Closes: lightning#448
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Co-Authored-By: rustyrussell <rusty@rustcorp.com.au>
@cdecker
Copy link
Collaborator

@cdecker cdecker commented Jan 22, 2019

Merging as decided during the 2019/01/21 IRC meeting.

Loading

@cdecker cdecker merged commit 1f4538c into lightning:master Jan 22, 2019
1 check was pending
Loading
t-bast pushed a commit that referenced this issue Mar 5, 2021
…used as tie-breaker for sorting (#539)

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 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants