sweep: start tracking input spending status in the fee bumper #9447
Conversation
yyforyongyu
added
utxo sweeping
no-itest
no-changelog
size/micro
|
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 (
|
yyforyongyu
force-pushed
the
yy-prepare-fee-replace
branch
from
e8cb0c7 to
a738e7f
Compare
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
from
48d4631 to
bd0f218
Compare
| continue | ||
| } | ||
|
|
||
| log.Warnf("Detected third party spent of output=%v "+ | ||
| "in tx=%v", op, spend.SpendingTx.TxHash()) | ||
| spendingTx := spend.SpendingTx |
There was a problem hiding this comment.
I wonder if we should actually block here, even if just for a moment, to allow the scheduler to run the goroutine that does the dispatch.
Spent a bit of time to re-familiarize myself with the notifier after the latest set of refactors, and I don't see an area where we'll insta dispatch the response before exiting the initial method call.
There was a problem hiding this comment.
you mean we perform a tiny sleep (sth likecase <- time.After(100ms)) instead?
We did refactor this area a bit here 1200b75, which makes sure the block is always sent before the tx, but we cannot guarantee the order is maintained since pipeline is a bit deep we cannot be sure they are read in this order.
A previous attempt was to implement a method HasOutpointSpent on Blockbeat - the idea is that, whenever we are notified of a block, we can easily access the block data to see if the watched outputs are spent or not, hence eliminating the race, which guarantees we won't miss a spending event. However there was some difficult involved when implementing it in neutrino, as discussed here. Now that you mention it I think it's still worthy to keep it as a TODO, since we can just register spend when it's neutrino to avoid fetching full blocks, and read the block data when it's a full node.
yyforyongyu
force-pushed
the
yy-prepare-fee-replace
branch
from
a738e7f to
b98542b
Compare
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
2 times, most recently
from
0ca8914 to
18df4fb
Compare
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
from
18df4fb to
af48039
Compare
There was a problem hiding this comment.
The code itself looks pretty good to me. Only nits there.
The first commit dcd5119 looks like it doesn't belong in this PR, since the added test isn't fixed until the next PR.
Also TxUnknownSpend is currently unhandled in UtxoSweeper, which means things would break if we merged this PR alone. We should really add a minimal switch case to UtxoSweeper.handleBumpEvent that keeps the existing behavior for TxUnknownSpend (i.e. same as TxFailed case).
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
2 times, most recently
from
cb67094 to
e2a7210
Compare
Moved to the next PR.
We can't merge this PR back to the master alone tho. The plan is to merge #9448 to this one, and then merge this one to the master, otherwise the itests would fail. Maybe I should've just created one PR instead - was thinking about reducing each PR's size, but the split could've done better I guess. |
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
from
e2a7210 to
7cdf369
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @morehouse and @yyforyongyu)
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
2 times, most recently
from
cfbf023 to
53f84ed
Compare
|
@morehouse: review reminder |
To track the input and its spending tx, which will be used later to detect unknown spends.
This commit refactors the `processRecords` to always handle the inputs spent when processing the records. We now make sure to handle unknown spends for all backends (previously only neutrino), and rely solely on the spending notification to give us the onchain status of inputs.
We now rename "third party" to "unknown" as the inputs can be spent via an older sweeping tx, a third party (anchor), or a remote party (pin). In fee bumper we don't have the info to distinguish the above cases, and leave them to be further handled by the sweeper as it has more context.
This commit adds a new field `InputsSpent` to the `BumpResult` so they can be used to track inputs spent by txns not recoginized by the fee bumper.
We now start handling `TxUnknownSpend` in our sweeper to make sure the failed inputs are retried when possible.
This is a minor refactor so the `createAndPublishTx` flow becomes more clear, also prepares for the following commit where we start to handle missing inputs.
A minor refactor to break the method `handleUnknownSpent` into two steps, which prepares the following commit where we start handling missing inputs.
This commit refactors `handleInitialTxError` and `createAndCheckTx` to take a `monitorRecord` param, which prepares for the following commit where we start handling missing inputs.
This commit handles the case when the input is missing during the RBF process, which could happen when the bumped tx has inputs being spent by a third party. Normally we should be able to catch the spend early via the spending notification and never attempt to fee bump the record. However, due to the possible race between block notification and spend notification, this cannot be guaranteed. Thus, we need to handle the case during the RBF when seeing a `ErrMissingInputs`, which can only happen when the inputs are spent by others.
This commit adds the failed tx to the result when marking the input as fatal, which is used in the commit resolver when handling revoked outputs.
Previously, when a given input is found spent in the mempool, we'd mark it as Published and never offer it to the fee bumper. This is dangerous as the input will never be fee bumped. We now fix it by always initializing the input with state Init, and only use mempool to check for fee and fee rate. This changes the current restart behavior - as previously when a sweeping tx is broadcast, the node shuts down, when it starts again, the input will be offered to the sweeper again, but not to the fee bumper, which means the sweeping tx will stay in the mempool with the last-tried fee rate. After this change, after a restart, the input will be swept again, and the fee bumper will monitor its status. The restart will also behave like a fee bump if there's already an existing sweeping tx in the mempool.
So we can focus on testing normal flow vs persistence flow.
Before this commit, the only error returned from `IsOurTx` is when the root bucket was not created. In that case, we should consider the tx to be not found in our db, since technically our db is empty. A future PR may consider treating our wallet as the single source of truth and query the wallet instead to check for past sweeping txns.
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
from
53f84ed to
7ab0e15
Compare
yyforyongyu
force-pushed
the
yy-sweeper-fix
branch
from
f8698a1 to
9f7e2bf
Compare
|
|
I think it's just that the available space on the GitHub runners is very low. So it's probably just the build cache getting too large with all the different commits being compiled one-by-one. I cleaned the GitHub cache and re-ran the step. |
Actually, turns out we're caching things twice, since |
There was a problem hiding this comment.
LGTM
Reviewed 6 of 12 files at r3.
Reviewable status: 7 of 13 files reviewed, 9 unresolved discussions (waiting on @morehouse and @yyforyongyu)
There are two places tracking the spending status of a given input - one in the sweeper, the other in the fee bumper. We now move the tracking to be handled in the fee bumper so we always have a single source of truth. By the end of this fix, we should see that,
The fix is made of two PRs to keep the size small - the first PR will enable tracking the spending status of inputs in the fee bumper, and the second will fix the rest.
Depends on,
FailedtoFataland minor refactor #9446This change is