Inflights ordering, crashfix #4521
Conversation
niftynei
force-pushed
the
nifty/rbf-err-fix
branch
from
d47f78a to
2a155fb
Compare
rustyrussell
force-pushed
the
nifty/rbf-err-fix
branch
from
2a155fb to
9fc5986
Compare
|
Trivial rebase, and added Changelog-Experimental line. Ack 9fc5986 |
|
Two outstanding CI problems; one is fixed in #4541, the other is in |
niftynei
force-pushed
the
nifty/rbf-err-fix
branch
from
9fc5986 to
476aead
Compare
An attempted + failed RBF results in a crashed/dead node
When we re-populate from disk, we need to know what order to recreate the inflights list in. Fixes ElementsProject#4511
niftynei
force-pushed
the
nifty/rbf-err-fix
branch
from
476aead to
a573d42
Compare
|
Found some more bugs in the RBF pathways. Here's some tests and fixes. ty @jsarenik for the help in identifying these! |
niftynei
force-pushed
the
nifty/rbf-err-fix
branch
2 times, most recently
from
04d4c9a to
ffdd029
Compare
|
@niftynei Thank you! My node is already running It is my pleasure to cause problems that lead to improvements. Thanks again for your care! ACK ffdd029 |
Changelog-Added: for v2 channels, we now list the inflights information for a channel
We don't pass the minimum fee requirement the first few times we attempt an RBF, so it fails. Keep going until actually replaces
If it's a unilateral close, we need to drop all the inflights also, as we don't know which of them ended up being mined.
Changelog-Added: EXPERIMENTAL JSON-RPC: `listpeers` now includes the `scratch_txid` for every inflight (if is a dual-funded channel)
If you close a channel, the state won't be DUALOPEND_AWAITING_LOCKIN
For convenience sake, include the inflight's signed txs as well
Otherwise we're missing info when we go to broadcast these and can't properly sign the transaction to close it. Found-by: @Jasan
Little check to make sure that we can produce a signed commitment tx for every inflight we've got saved.
If you drop an rbf'd channel to chain (before any updates have been made) we should drop *all* of the inflights to chain
This assertion is not valid if a non-last funding tx is mined
If the peer is offline when we see the funding txid, we don't actually update the channel's info. Here, we move it up to where the scid is set, so that we always update the channel's funding_txid to the correct (mined) information.
The channel's open has been mined, we don't need to keep all of these around now.
We weren't watching for all inflights after the node is restarted. Yikes.
log when we're telling the peer our depth's been reached
We weren't testing that a non-tip inflight would get opened correctly. Until now, that is.
niftynei
force-pushed
the
nifty/rbf-err-fix
branch
from
ffdd029 to
2d4f3d0
Compare
|
@rustyrussell Excuse me, I might have been too fast in ACKing it, see below. @niftynei now I realized that the transaction the fixed version did is actually not pointing to any address of mine, but rather an other 2of2 multising (guessing from the length of bech32). Where is it going? And where are the 330 sats going? I will send a link to the TX in question on-request. |
|
@rustyrussell UPDATE: Everything is OK. Learning on the run, with And the 330 sats UTXO is called |
Found a number of issues with the RBF path for the v2 channel opens. Namely, we weren't handling the inflight promotion and broadcast correctly if a non-last transaction ended up being mined (which is very likely given how RBF attempts can fail if they don't pass bitcoind's RBF rules for fees).
We weren't ordering channel_inflights, so they get out of order when reloaded from disk, which fails an assert and leads to a crash loop. Resolved by ordering inflights by feerate.
Fixes #4511