Fix handling of CLTVs: in three ways!#2043
Closed
rustyrussell wants to merge 8 commits into
Closed
Conversation
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I audited the callers. So remove the code which tested this. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This was suggested by Pierre-Marie as the solution to the 'same HTLC, different CLTV' signature mismatch. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ure. Create a second HTLC with a different CTLV but same preimage; onchaind uses the wrong signature and fails to grind it. Reported-by: molz (#c-lightning) Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We set up HTLCs with the same preimage and both different and same CLTVs in both directions, then make sure that onchaind is OK and that the HTLCs are failed without causing downstream failure. We do this for both our-unilateral and their-unilateral cases. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
… closing. If there are two HTLCs with the same preimage, lightningd would always find the first one. By including the id in the `struct htlc_stub` it's both faster (normal HTLC lookup) and allows lightningd to detect that onchaind wants to fail both of them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we have multiple HTLCs with the same preimage and the same CLTV, it doesn't matter what order we treat them (they're literally identical). But when we offer HTLCs with the same preimage but different CLTVs, the commitment tx outputs look identical, but the HTLC txs are different: if we simply take the first HTLC which matches (and that's not the right one), the HTLC signature we got from them won't match. As we rely on the signature matching to detect the fee paid, we get: onchaind: STATUS_FAIL_INTERNAL_ERROR: grind_fee failed So we alter match_htlc_output() to return an array of all matching HTLC indices, which can have more than one entry for offered HTLCs. If it's our commitment, we loop through until one of the HTLC signatures matches. If it's their commitment, we choose the HTLC with the largest CLTV: we're going to ignore it once that hits anyway, so this is the most conservative approach. If it's a penalty, it doesn't matter since we steal all HTLC outputs the same independent of CLTV. For accepted HTLCs, the CLTV value is encoded in the witness script, so this confusion isn't possible. We nonetheless assert that the CLTVs all match in that case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/find-onchain-bug
branch
from
4863e11 to
4c480e9
Compare
Contributor
Author
|
Github weirdness due to outage. Closing to reopen. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First, make sure we spell it correctly! Then, fix channeld's HTLC signature confusion (thanks to lightning/bolts#491). Then, fix onchaind's HTLC confusion.
This is all due to a great bug report by molz on #c-lightning on Freenode.