Repository navigation
Sweep a colored output when its closing transaction's witness updates, whatever other witnesses fail - #208
Open
darkhorse-core wants to merge 3 commits into
Conversation
…, whatever other witnesses fail
…coind running without txindex
Author
|
Added a test to reproduce this, details in the description. |
There was a problem hiding this comment.
Hi @darkhorse-core, thanks for the contribution & for adding a test that reproduces the issue! ACK from inspection that this is indeed an issue.
General feedback:
- Because this impacts RGB-Tools' branch as well, it would be best to move this upstream. They might have further feedback on the fix approach as well
- If you can spend a bit more time trying to convey the issue at a high-level next time, that might help with review. :) Took me a while to understand the bug. If using AI, asking it for a more succinct description can sometimes help. IIUC, it seems like the main points are:
- To replicate this, must run electrs with txindex=0 (which is the default mode, but notably using this mode was not supported in the tests before his test addition)
- If specifically a colored channel open fails... this leads to odd behavior. Every later colored channel close leaves the node's funds stuck on chain, for closes completely unrelated to that failed open.
- The closing transaction confirms on chain as normal. What fails is the sweep afterward, where the node moves its BTC and RGB from the closing output back into its wallet. That step keeps failing on every retry, so the funds are stuck there, though not lost.
- If you can squash the two fix commits into one, that also helps us with review. But keep the test in a separate commit
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
We run a node on signet against our own electrs. Its bitcoind runs with the default
txindex=0, which electrs supports since 0.9. Two colored opens that were discarded before broadcast left the node-side outputs of three later closed channels unswept for two days; with this change the next retry swept all three and the RGB amounts settled.Problem
RgbOutputSpender::spend_spendable_outputs(src/ldk.rs) callsupdate_witnesses(closing_height, [closing txid])and gives up whenupdate_res.failedis not empty.update_witnessesalso goes over the other witnesses in the stock that are notIgnored, so one failing witness blocks the sweep even when the closing transaction updated fine. LDK retries every minute and fails the same way, and the node's BTC and RGB amount stay on the closing output.It happens when both hold:
Event::DiscardFunding): it is upserted asTentative, andhandle_open_chan_faildoes not archive it.No such mempool or blockchain transactionwith it,No such mempool transaction. Use -txindex ...without it. The electrum resolver treats only the first asUnresolved, so the second becomes aResolverIssueand lands infailed.Sweeping a closing output should not depend on whether unrelated witnesses resolve. The txindex setting only decides whether this shows up; the witness that fails is one the node itself left behind in condition 1.
Fix
Fail the sweep only when the closing transaction's own witness failed to update.
This keeps sweeping working on any electrs setup, whatever its bitcoind's txindex setting, so node operators do not need to know that this setting matters here. Treating bitcoind's non-txindex message as
Unresolvedin the electrum resolver would also cover it, on the rgb-ops side.Test
sweep_after_discarded_fundingrecreates bitcoind with-txindex=0(optionalBITCOIND_EXTRA_ARGSincompose.yaml), discards a colored open withDROP_FUNDING_SIGNED_ON_NODE, then opens and cooperatively closes another colored channel and waits for node1's output to be swept.On master 7597bbd it fails without the fix and passes with it. Without the fix and with txindex on it passes, which is why the current suite does not catch this. dev's BFA dependencies are not public, so the test has not been compiled against dev; on this branch it uses
open_channel_request_rawand thebitcoind()helper instead of the master equivalents.