Wait for 'FUNDING_LOCKED' in dualopend, not channeld #4293
Conversation
niftynei
force-pushed
the
nifty/df-rbf
branch
2 times, most recently
from
31a33e4
to
96510ad
Compare
There was a problem hiding this comment.
Verified up until commit 9e151f4.
Since we change the state machine to use two new states that might have been persisted in the DB, I'm a bit worried that we might break channels that were initiated before the upgrade, then loaded after the upgrade and no longer functional. Specifically I'm thinking about CHANNELD_AWAITING_LOCKIN vs. DUALOPEND_AWAITING_LOCKIN.
Can you confirm that this has been taken care of and works as expected?
I'll finish up the review asap, but thought some early feedback might be good 🎄🎅
|
Lots of great feedback here, thanks!
I don't believe this is a necessary concern, given that as it stands currently in Given that the code is 'still dark', do you think it's reasonable to add handling/test for this case? |
|
Thanks @niftynei for the update, I went through the range-diff and marked the addressed feedback as resolved. The only ones that are not addressed yet are:
Could you comment on those? |
|
btw, the size of the PR made me update my range-diff tool so I can navigate it easier and with colors xD |
|
ACK 2d82765 |
|
Rebased on top of ACK cbcddf2 |
cdecker
force-pushed
the
nifty/df-rbf
branch
2 times, most recently
from
bdaa094
to
6fb51d5
Compare
Non-functional yet, but this gets all the pieces in the right places, rips the signature signing functionality out of channeld.
Suggested-By: Christian Decker (@cdecker)
We weren't sending a channel_open notification for dual-funded channels. This is only sent for the 'accepter' side. We send it as soon as both funding_tx sigs have been exchanged, even though it's possible the funding transaction might be published without this having been the case. Since we fail the channel if this happens, only notifying for good/valid channels reaching the broadcast state is the right way to handle this.
Move pid collection down to when dualopend is definitely dead
We don't need it anymore. Normally it gets cleaned up by `cmd` but we're done with it here.
we got rid of push_msats for dual funded channels. this assumes that hte peer will match an equal amount of sats as ours (the df_accepter.py plugin will do this)
Method for setting a ceiling on the fund matching capabilities of df_accepter. Setting it to zero means we don't fund the channel anymore.
nodes need to be using the accepter plugin, so they'll match funding. we used to use push_msat for this, but v2 gets rid of it. *sad trombone*
Test connection/reconnection handling for v2 opens. We needed to fixup the accepter plugin so that we were freeing up inputs on disconnect/failure.
We're still ignoring multifundchannel failure tests, but now we use the 'dual-fund' flag instead of a blanket true.
Reject a peer's request to open a channel while we're already in progress
|
Tiny fixup to address a python lint. Merging asap :-) |
|
ACK 92b0337 |
Prework for allowing RBF of dual funded channel opens. We need to wait for FUNDING_LOCKED in dualopend, so that handling a request to RBF the channel can transition smoothly back to the 'opening' message flow.
To do this, we move all of the tx_sigs + psbt handling out of channeld, back into dualopend. We also add handling for reconnections, shutdown requests, and waiting for FUNDING_LOCKED to dualopend.
This looks like a big PR (because it is), but most of the stuff is small tweaks to the test suite to have tests pass for dual-funded channel flows. These aren't turned on here; waiting for the move over to dual-funding being a runtime flag (instead of compile flag).
Update: now based on top of #4294.
Changelog-None