Channel: improve force-close handling #184
Conversation
aarani
changed the title
aarani
force-pushed
the
fundingTxSpentRefactorBackport
branch
from
b17c4ba
to
c92d94a
Compare
| |> ignore) | ||
|
|
||
| // FIXME: what if all the money is in htlcs? | ||
| if toLocalIndexOpt.IsNone && toRemoteIndexOpt.IsNone then |
There was a problem hiding this comment.
This is a temporary fix for the false-positive scenario, this causes the function to return "Unknown Closing Tx" if we couldn't find any to_local and to_remote output, but this creates an issue with commitment TXs that only have HTLC outputs and not any main one.
There was a problem hiding this comment.
I want you to add test to check the false positive scenario.
but this creates an issue with commitment TXs that only have HTLC outputs and not any main one.
And a test to check that this won't lead to the crash?
There was a problem hiding this comment.
I want you to add test to check the false positive scenario.
I added a test for old local commitment txs
And a test to check that this won't lead to the crash?
I think I should explain more, there's no chance of a crash, the only problem with that edge case is that instead of BalanceBelowDustLimit error we return UnknownClosingTx which seems a bad error for the case (I added a comment explaining it in the code also)
| MainOutput = Error UnknownClosingTx | ||
| } | ||
|
|
||
| let HandleFundingTxSpent |
There was a problem hiding this comment.
I think this function needs test?
There was a problem hiding this comment.
I added a test for revoked/remote/local cases.
|
some @aarani 's questions are hard hard for me to answer. And I have no time to catch up. (Channel state handling is not my interest anyway.) |
aarani
force-pushed
the
fundingTxSpentRefactorBackport
branch
2 times, most recently
from
5bf2211
to
abf5b58
Compare
|
How's this looking Joe? @canndrew hey hello, by any chance you can have a quick look in case you spot something off very quickly? |
| lastLocalCommitmentSpec.ToRemote.ToMoney() | ||
|
|
||
|
|
||
| Expect.equal outputsSumFromRemoteSpendingRevokedRemoteCommitment (expectedAmountFromToRemote + expectedAmountFromToLocal) "wrong penalty tx" |
There was a problem hiding this comment.
You are running 3 attempt for HandleFundingTxSpent, and setup and assertions are mostly the same, it can be more succinct by grouping into the function instead of repeating.
There was a problem hiding this comment.
I grouped validation, commitment tx creation (as much as I could) into functions
aarani
force-pushed
the
fundingTxSpentRefactorBackport
branch
from
abf5b58
to
4acb992
Compare
aarani
force-pushed
the
fundingTxSpentRefactorBackport
branch
from
4acb992
to
78882d4
Compare
This commit creates a unified function to handle most force-close cases (last remote commit, next remote commit, last local commit, remote revoked commits), this removes the need to implement this logic in the upper layer. This commit also fixes a bug where revocation was broken if local's to_self_delay <> remote's to_self_delay.
knocte
force-pushed
the
fundingTxSpentRefactorBackport
branch
from
a806702
to
a19f5d2
Compare
Rebasing from github UI (no changes). |
This commit creates a unified function to handle most force-close
cases (last remote commit, next remote commit, last local commit,
remote revoked commits), this removes the need to implement this
logic in the upper layer.
This commit also fixes a bug where revocation was broken if
local's to_self_delay <> remote's to_self_delay.