-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Beat [1/4]: handle sweeper's broadcast error #8893
base: yy-feature-blockbeat
Are you sure you want to change the base?
Beat [1/4]: handle sweeper's broadcast error #8893
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Did a quick drive-by review to load up on context. Not super familiar with the new sweeper system, so best not to count this review at all. I'm mostly interested in the block beat implementation itself (which seems to be part 2 of 3).
263006e
to
4630d4f
Compare
a0393a0
to
9d6f8e7
Compare
388981d
to
adca500
Compare
9d6f8e7
to
7183080
Compare
adca500
to
c5e6b49
Compare
7183080
to
8b95551
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high level pass looks good! I need to spend a bit more time getting familiar with sweeper stuff on the next pass though
log.Debugf("Removing confirmed monitor record=%v, tx=%v", id, | ||
result.Tx.TxHash()) | ||
|
||
case TxError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the select's comment should probably be updated explaining this third case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool updated the docs to make it more clear.
sweep/fee_bumper.go
Outdated
@@ -124,6 +124,10 @@ type BumpRequest struct { | |||
// StartingFeeRate is an optional parameter that can be used to specify | |||
// the initial fee rate to use for the fee function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may help with review just to say in the commit msg why it will not work with the upcoming change? just cause this commit still does allow immediate broadcast if Immediate
is true so it is unclear just looking at this commit why that then would work later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool updated!
inp := createTestInput(1000, input.WitnessKeyHash) | ||
|
||
// Create a testing bump request. | ||
req := &BumpRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a test case for when BumpRequest.Immediate is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, added!
1e898e2
to
4548971
Compare
8b95551
to
9ec032a
Compare
e0ec893
to
9180762
Compare
Aarrgh.... Sorry, I deleted the base branch before GitHub could re-base it... |
Testing re-open |
9ec032a
to
436aece
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main question and/or concern is with the first commit, that could have a big negative impact on Neutrino. Not sure how consequential that is for the rest of the series of PRs?
The rest looks good to me 🎉
startingHeader, err := n.p2pNode.GetBlockHeader( | ||
&startingPoint.Hash, | ||
) | ||
startingBlock, err := n.p2pNode.GetBlock(startingPoint.Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, doesn't this change mean that in a Neutrino environment we're suddenly fetching way more blocks (if not all) from our peers instead of just the header that we have anyway?
Maybe we need to find some middle ground where we have a new struct that has the hash, height and header and then fn.Option[Block]
that is fn.None
for Neutrino. Then when we decide we actually want to fetch the block we do it lazily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it means we will fetch the block once from one of the peers then it will be cached? Will investigate more.
sweep/fee_bumper.go
Outdated
t.records.ForEach(visitor) | ||
|
||
// Handle the initial broadcast. | ||
for requestID, r := range initialRecords { | ||
rec := r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed anymore. First, because we're using Go 1.22 and second because the loop variable is passed to a method call, so it's placed on the stack anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed - also added a commit to remove other occurrence
@@ -178,7 +178,8 @@ func (b *BitcoindNotifier) startNotifier() error { | |||
if err != nil { | |||
return err | |||
} | |||
blockHeader, err := b.chainConn.GetBlockHeader(currentHash) | |||
|
|||
block, err := b.GetBlock(currentHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between b.GetBlock
and b.chainConn.GetBlock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.GetBlock
wraps the b.chainConn.GetBlock
inside a cache.
startingHeader, err := n.p2pNode.GetBlockHeader( | ||
&startingPoint.Hash, | ||
) | ||
startingBlock, err := n.p2pNode.GetBlock(startingPoint.Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this skips the locking that's normally done in n.GetBlock
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with n.GetBlock
func (n *NeutrinoChainConn) GetBlock( | ||
blockHash *chainhash.Hash) (*wire.MsgBlock, error) { | ||
|
||
utilBlock, err := n.p2pNode.GetBlock(*blockHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be a locking problem here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate a bit? Think the lock is only used in NeutrinoNotifier
tho
if req.Immediate { | ||
t.handleInitialBroadcast(record, requestID) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for the typical case (Immediate: false
) we now wait an extra block before broadcasting? We already wait one block for the sweeper to process the transaction, and now this waits another before broadcasting...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually save two blocks (worst case) when sweeping inputs now. So this PR is some commits cherry-picked from the final PR so it may not make sense to look at it alone, but the full pic is, suppose a force close happens at block X, contractcourt
will process block X, prepare all the necessary resolutions, offer them to the sweeper. Then sweeper
processes block X by broadcasting the sweeping txns.
sweep/fee_bumper.go
Outdated
// Create a tx so the caller knows which inputs have failed. | ||
sweepTx := wire.NewMsgTx(2) | ||
for _, o := range r.req.Inputs { | ||
sweepTx.AddTxIn(&wire.TxIn{ | ||
PreviousOutPoint: o.OutPoint(), | ||
Sequence: o.BlocksToMaturity(), | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems hacky to create a fake tx to tell the caller info it should already know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the flow here - we now no longer need to get the inputs from the tx!
event = TxFailed | ||
|
||
default: | ||
event = TxError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what circumstances would we hit this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the comments
@@ -221,6 +221,30 @@ func (p *SweeperInput) terminated() bool { | |||
} | |||
} | |||
|
|||
// isMature returns a boolean indicating whether the input has a timelock that | |||
// has been reached or not. The locktime found is also returned. | |||
func (p *SweeperInput) isMature(currentHeight uint32) (bool, uint32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do timelocked inputs get to the sweeper? I thought they were only supposed to be offered to the sweeper once timelocks are fulfilled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's changed in a later PR.
436aece
to
f6c5de6
Compare
@ellemouton: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yyforyongyu - am assuming this is not yet ready for another round yeah?
Thanks for checking in! Nope not yet, will request once ready! |
So the block data can be used by subsystems without calling `GetBlock`.
Also updated the loggings. This new state will be used in the following commit.
This prepares the following commit where we now let the fee bumpr decides whether to broadcast immediately or not.
This commit changes how inputs are handled upon receiving a bump result. Previously the inputs are taken from the `BumpResult.Tx`, which is now instead being handled locally as we will remember the input set when sending the bump request, and handle this input set when a result is received.
This commit adds a new method `handleInitialBroadcast` to handle the initial broadcast. Previously we'd broadcast immediately inside `Broadcast`, which soon will not work after the `blockbeat` is implemented as the action to publish is now always triggered by a new block. Meanwhile, we still keep the option to bypass the block trigger so users can broadcast immediately by setting `Immediate` to true.
Previously in `markInputFailed`, we'd remove all inputs under the same group via `removeExclusiveGroup`. This is wrong as when the current sweep fails for this input, it shouldn't affect other inputs.
Also updated `handlePendingSweepsReq` to skip immature inputs so the returned results are the same as those in pre-0.18.0.
f6c5de6
to
0f280d2
Compare
Depends on #8892.
This PR prepares the incoming blockbeat PRs, the changes are,
BlockEpoch
now has the block data instead of the block header. This block data is used in the incoming blockbeat PR to query spending transactions.TxError
. Inputs resulting in this state will be removed from the sweeper.