multi: preliminary changes towards integrating the new RBF close protocol feature #9559
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Looking good! Since we are merging to a side branch, I think we can skip the itest CI as long as they are green in the final PR, as they are likely to only start working once all pieces are fit together. I think once the linter and check commits are fixed we are good to go - the unit tests are failed due to compile error so they are expected to pass once the linter is fixed.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, 5 of 5 files at r10, 4 of 4 files at r11, 3 of 3 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Crypt-iQ)
lnwire/features.go line 236 at r1 (raw file):
KeysendOptional = 55 // RbfCoopCloseRequired is an required feature bit that signals that
nit: an required -> a required
lnwire/features.go line 244 at r1 (raw file):
RbfCoopCloseOptional = 61 // RbfCoopCloseRequiredStaging is an required feature bit that signals
nit: an required -> a required
lnwire/features.go line 248 at r1 (raw file):
RbfCoopCloseRequiredStaging = 160 // RbfCoopCloseOptionalStaging is a optional feature bit that signals
nit: a optional feature bit -> an optional feature bit
peer/daemon_adapters.go line 79 at r6 (raw file):
// gossip network. // //nolint:iface
looks like we have two interfaces which are the same? There's already a linkNetworkController in chan_observer.go
peer/chan_observer.go line 98 at r5 (raw file):
} _ = l.link.DisableAdds(htlcswitch.Incoming)
nit: add a debug/warning log so we know whether it's disabled or not?
peer/chan_observer.go line 151 at r5 (raw file):
snapshot := l.chanView.StateSnapshot() return fn.Some[chancloser.ShutdownBalances](
nit: can remove chancloser.ShutdownBalances,
return fn.Some(chancloser.ShutdownBalances{
LocalBalance: snapshot.LocalBalance,
RemoteBalance: snapshot.RemoteBalance,
})
Cherry-pick + rebase issue. Should be fixed in a push shortly. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yyforyongyu)
peer/chan_observer.go line 151 at r5 (raw file):
Previously, yyforyongyu (Yong) wrote…
nit: can remove
chancloser.ShutdownBalances,return fn.Some(chancloser.ShutdownBalances{ LocalBalance: snapshot.LocalBalance, RemoteBalance: snapshot.RemoteBalance, })
Nice this PR was before they improved the type inferencing.
peer/daemon_adapters.go line 79 at r6 (raw file):
Previously, yyforyongyu (Yong) wrote…
looks like we have two interfaces which are the same? There's already a
linkNetworkControllerinchan_observer.go
Removed this one.
In an earlier version the state machine executor also gained this ability as a daemon event, but reviewer feedback was that is less universal. So moved it into the env of the state machine instead.
Roasbeef
force-pushed
the
rbf-staging-prep
branch
from
7159ec2 to
4120d62
Compare
Roasbeef
force-pushed
the
rbf-staging-prep
branch
from
4120d62 to
95f1985
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Roasbeef)
lnwire/features.go line 236 at r14 (raw file):
KeysendOptional = 55 // RbfCoopCloseRequired is n required feature bit that signals that
Suggestion:
a requiredlnwire/features.go line 244 at r14 (raw file):
RbfCoopCloseOptional = 61 // RbfCoopCloseRequiredStaging is n required feature bit that signals
Suggestion:
a requiredIn this commit, we change LinkDirection to be a type alias. This makes creating wrapper structs/interfaces easier, as we don't need to leak htlcswitch.LinkDirection, instead we can accept a bool.
In this commit, we add an implementation of a new interface the rbf coop state machine needs. We take care to accept interfaces everywhere, to make this easier to test, and decouple from the concrete types we'll end up using elsewhere.
Similar to the last commit, in this commit, we make a daemon adapter impl for lnd. We'll use this for any future protofsm based state machine.
We don't return an error on broadcast fail as the broadcast might have failed due to insufficient fees, or inability to be replaced, which may happen when one side attempts to unnecessarily bump their coop close fee.
With this commit, we make sure we set the right height hint, even if the channel is a zero conf channel.
In this commit, we update `chooseDeliveryScript` to generate a new script if needed. This allows us to fold in a few other lines that always followed this function into this expanded function. The tests have been updated accordingly.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, 1 of 1 files at r8, 2 of 2 files at r9, 4 of 5 files at r10, 3 of 4 files at r11, 3 of 3 files at r12, 1 of 2 files at r13, 7 of 7 files at r14.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yyforyongyu)
lnwire/features.go line 236 at r14 (raw file):
KeysendOptional = 55 // RbfCoopCloseRequired is n required feature bit that signals that
Done.
lnwire/features.go line 244 at r14 (raw file):
RbfCoopCloseOptional = 61 // RbfCoopCloseRequiredStaging is n required feature bit that signals
Done.
Roasbeef
force-pushed
the
rbf-staging-prep
branch
from
95f1985 to
0423466
Compare
|
Merging into side feature branch. |
Roasbeef
merged commit d109107
into
lightningnetwork:rbf-staging
This PR contains a set of preliminary changes towards integrating the RBF close state machine.
Spun off from: #8453
NOTE: the merge target here is a side feature branch:
rbf-staging.This change is