WIP: Deal with confusion with HTLCs with same preimage, esp. different CLTVs.#2044
Merged
cdecker merged 9 commits intoOct 23, 2018
Merged
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>
Member
|
Will review this asap. In the meantime there is null-pointer somewhere: |
Contributor
Author
|
Really need a backtrace on that one... Esp. which json command it was. That assertion is caused by us trying to json_stream_success() or json_stream_fail() twice on the same cmd. OK, got it thanks! I've appended a fix for that one... |
rustyrussell
force-pushed
the
guilt/find-onchain-bug
branch
from
4c480e9 to
c2effb2
Compare
Contributor
Author
|
Travis was testing old branch, so I removed a '.' from title of final commit and force-pushed to trigger it again. |
rustyrussell
changed the title
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>
In e46ce0f I accidentally removed the actual code which fails the command. As a result, if we retry and it succeeds later, we can end up "succeeding" the started-failing command, causing us to hit the 'assert(!cmd->have_json_stream);' in new_json_stream. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/find-onchain-bug
branch
from
a523b50 to
34cd9ff
Compare
cdecker
approved these changes
Oct 23, 2018
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
2 participants
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, who reported that this fixed his issue.