Keep the wallet Bitcoin coins instead of rediscovering them - #189
Open
albertodeluigi wants to merge 17 commits into
Open
Keep the wallet Bitcoin coins instead of rediscovering them#189albertodeluigi wants to merge 17 commits into
albertodeluigi wants to merge 17 commits into
Conversation
changeEvent() answers a palette change by re-colouring the four sidebar icons. The actions it names are created in createActions(), which the constructor reaches long after RestoreWindowGeometry() -- and restoring a geometry pumps events. A palette change that lands in between finds four null pointers and takes the process down inside QAction::setIcon, before the log file is open: an access violation with nothing written anywhere. On Windows this is not a rare race. The shell hands the theme to a process it activates, so launching from Explorer -- double-clicking the exe, which is how anyone actually starts a wallet -- crashed 5 times out of 5, while the same binary started from a shell survived 5 out of 5. The report is a 0xc0000005 at a fixed offset, five bytes into QAction::setIcon, which is the prologue dereferencing this. Guard each pointer. With the guard, 5 launches out of 5 from Explorer reach a window.
bitcoind runs one scantxoutset at a time for the whole process, and this file has three callers: the Bitcoin balance every loaded wallet refreshes on its own timer, a Bitcoin send, and the parent-chain history. Two wallets open in one GUI are enough to break it: their timers start together, so the two balance refreshes land in the same second, once a minute, for ever. The loser was handed bitcoind refusal and the GUI printed it verbatim -- "Scan already in progress, use action abort or status", advice addressed to nobody, parked under the balances until a refresh happened to find the slot free. On this machine it never did. The send side already waited for the slot. Share that wait with the other two, and make it a DEADLINE rather than a count of tries: a testnet4 scan measured 18.8 s over 14.2 million outputs, and a budget shorter than one scan means the second caller always gives up seconds before the slot frees. 90 s covers a scan several times that size and still bounds the wait, so an abandoned scan cannot hang a wallet. This is a mitigation, not the cure: two wallets still pay for two full scans a minute for a number that rarely moves. One scan per process, or a shared cached result, is the real fix and a larger change.
Every reading of a parent-chain balance costs a full scantxoutset: measured 18.8 s over 14.2 million outputs on testnet4, and a mainnet-sized UTXO set is over an order of magnitude larger -- minutes per reading, on a refresh that ran every 60 seconds per open wallet. Most of that work confirmed that nothing had changed. The node already walks every new parent block, output by output, looking for PoS checkpoints. Hand it the scripts and outpoints a wallet cares about and the same walk answers a second question for free: did this block pay, or spend, anything of ours. The count rides to the GUI inside getanchorstatus, which the overview polls every tick anyway, so knowing costs no RPC at all. - anchor: a watch list (raw scripts and outpoints) and a monotonic touch count, checked during the existing block walk. The output test sits BEFORE the OP_RETURN filter -- a payment to us is an ordinary output -- and the walk now also reads vin, because a spent coin moves a balance as much as a received one and is invisible otherwise. - wallet: getbtcbalance registers what to watch after a scan that succeeded. A failed scan knows nothing and must not replace what was known. - qt: refresh when the count moves, at most once a minute, plus one scan every ~30 minutes regardless -- for a node not walking blocks, a wallet with more UTXOs than the list holds, and the cold start before the first scan. - rpc: getbtcscanprogress, so the first scan of a wallet can show how far it has got instead of an empty line for minutes. Steady state goes from one full scan a minute per wallet to none.
The wallet held no record of its parent-chain coins. Every balance, and every send, rebuilt the answer from nothing with a scantxoutset over the entire parent UTXO set: 18.8 s across 14.2 million outputs on testnet4, minutes on a mainnet-sized set. That single fact caused everything reported today -- a balance that took twenty seconds, a Send window frozen twice while it asked the same question again, and a second send refused by Bitcoin as a replacement of the first, because a scan sees CONFIRMED coins only and so offered the coin the first send had already committed. Every serious wallet keeps what it owns and edits it as blocks arrive. Sequentia assets have always worked that way, which is exactly why they were always instant. This gives Bitcoin the same treatment. - parent_coins.json beside the wallet: the coins, the height and hash they were current at, and any pending spend of ours. The hash is what makes a parent reorg visible instead of silently describing a chain nobody is on. - Filled by one full scan, then advanced block by block, in both directions: a spent coin moves a balance as much as a received one. More than 500 blocks behind, or across a reorg, one full scan is the faster answer, and the code says so instead of guessing. - getbtcbalance answers from the record: measured 33.6 s -> 80 ms -> 55 ms on the same wallet, same coins, same totals to the satoshi. - A send picks from the record, marks what it committed, and adds its change at height 0, so the next send neither collides with the last nor waits for a confirmation. - getbtcfeerate, and a Bitcoin fee panel in Send. The asset fee panel is hidden for bitcoin because it prices assets, which left no fee controls at all: Recommended now shows the number -- testnet4 quoted 362 sat/vB, which is 0.0005 BTC of fee on a 0.01 BTC send -- and Custom lets it be refused. - Bitcoin DEFAULT_TRANSACTION_MAXFEE ceiling, for bitcoin sends only. Sequentia dropped the default ceiling because fees there may be paid in any asset; this fee is bitcoin, and the estimator behind it can be absurd.
Every parent RPC opens its own TCP connection and authenticates again, so the cost of catching up is counted in round trips, not in blocks. Asking for a height whose hash the previous block already told us is a whole connection spent on nothing: 500 blocks behind meant 1000 of them. Follow the nextblockhash a block already carries. Same blocks, same order, half the connections -- and the half that remains is the half that returns data we do not already have. Upstream 24.7.5 adds CallMainChainRPCBatch, which is the real answer for the catch-up path; this stands on its own until we rebase onto it.
With Bitcoin Core stopped, getbtcbalance answered 0.00000000. The record beside the wallet held three coins at the time; the wallet reported none of them, because the daemon it wanted to ask was not there. A zero balance reads as theft. A dated one reads as weather. When the parent chain cannot be reached, answer from the record and mark the answer stale: the coins existed at parent_height and nothing has been seen since. The overview says which block the figures are from and that Bitcoin is not answering, instead of replacing them with a zero. Falling through to a full scan here was pointless anyway -- it needs the same daemon. This is the other half of what a local record is for. It is not only faster than asking; it is what lets a wallet keep working when there is nobody to ask.
The catch-up limit was 500 blocks, chosen before anything had been measured. Then Bitcoin Core was stopped for a while and the wallet had 38 blocks to apply: it took 33.5 s, about 0.9 s a block. A full scan of the same wallet takes 33.6 s. So the crossing point was around forty blocks, and the limit as written would have spent seven and a half minutes catching up to avoid half a minute of scanning. The cost per block is the connection, not the block -- every parent RPC opens a fresh TCP connection and authenticates again. The cost of a scan is the size of somebody else s UTXO set. Neither is a constant this code can know, and they move in different directions between testnet4 and mainnet. So measure one and compare: the record now keeps how long its own last full scan took, and catching up is chosen only while it is cheaper. Batched parent RPC (upstream 24.7.5) will cut the per-block side sharply; this arithmetic follows it without being edited.
Three faults with one root: a send that has been broadcast but not mined is a state this code kept failing to represent. - A rebuild of the coin record dropped it. A scan sees the CONFIRMED chain, so it re-offered the coin an unconfirmed send had already committed and knew nothing of that send change. Any rebuild -- migration, reorg, realignment -- silently re-armed the double spend the record exists to prevent. A rebuild now carries both across. - Its confirmations read as unknown. They were inferred by asking whether an output of the transaction still existed; once a later send spent those outputs the answer said nothing, so a transaction with 42 confirmations was reported as unknown minutes after its change was spent. The record holds the height of every coin it has seen; ask it. - It looked broken. Zero confirmations drew transaction_0, the question mark that means nobody can vouch for this state, where the clock was meant. The question mark now appears only when the count really cannot be established. Also: a send rings the watch bell itself. It happens in a mempool, no block walk can see it, and the overview would have shown the spent coin until the next block or the half-hour safety refresh.
Typing an amount in tBTC put an error dialog on screen: No asset provided for recipient. Nothing to do with sending -- the send was never reached. refreshTxSize runs 400 ms after the last keystroke and asks the wallet to build the transaction, because the only honest size is the one it would really build. For a Bitcoin recipient that transaction cannot exist: bitcoin does not travel in a Sequentia transaction, its asset is the null one by design, and the wallet answers exactly that. The probe treats a failure as no total and moves on -- but WalletModel::prepareTransaction shows the wallet error to the user before returning, so the failure was not silent, and every keystroke produced a modal error for anyone typing an amount in bitcoin. Skip Bitcoin recipients in the probe, as the real send already does by taking the parent-chain road. The dialog was a nuisance rather than a blocker, which is why a send still went through after dismissing it.
A functional test for the record behind the Bitcoin balance: it is built on first use, moved forward over new blocks without sweeping the UTXO set again, sees money arrive, survives a restart, is rebuilt rather than walked across a parent reorg, and answers from what it knows -- marked stale -- when the parent chain cannot be reached. Writing it found two real faults: - The balance was read from a field whose NAME depends on which daemon answers: bitcoind says total_amount, an Elements-style daemon says total_unblinded_bitcoin_amount. Against the latter the wallet reported zero next to a list of coins plainly present. The total is now summed from the coins being recorded, so the balance and its own coins cannot disagree, and the field name stops mattering. - Short gaps were rescanned rather than walked whenever a scan happened to be quicker, which on a small chain is always. But walking is not merely faster: a scan sees only the confirmed chain and drops what only the record knows -- coins committed to a send still in a mempool, and its change. A wallet that rescans because rescanning is cheap would keep forgetting its own pending sends. Gaps under twenty blocks are now walked whatever the clock says. Not covered here, and said so in the file: sending. sendbtctoaddress builds a Bitcoin transaction, and the daemon standing in for Bitcoin in this framework is an Elements one that cannot decode it. Those three properties were exercised by hand on testnet4 instead.
The Send form has an Add Recipient button, and for bitcoin it led to a refusal:
one recipient, no other assets. Half of that was a real constraint and half was
ours. Bitcoin and a Sequentia asset genuinely cannot share a transaction -- two
chains -- but several Bitcoin recipients are an ordinary Bitcoin transaction
with several outputs, and sending them together costs one fee and one set of
inputs instead of one of each.
sendbtctoaddress now takes either an address and an amount, as before, or a
list of {address, amount}. The change output moves to the end, after the
recipients, and the record follows it there: getting that index wrong would
have lost track of our own change again.
subtractfeefromamount stays limited to a single recipient. With one the
deduction is unambiguous; with several it is a decision about whose payment
shrinks, and the wallet should not make it quietly.
The GUI says it earlier, too. Mixing chains now shows a warning in the form
while it is being filled, rather than waiting for Send to refuse a form the
user has already finished typing -- and the refusal, when it comes, says the
true reason instead of the old message that blamed the recipient count.
The Sequentia path already handled several recipients; nothing there changed.
The multi-recipient send recorded one address and one total, so the history showed a single row of -0.15 next to the first destination and the other two payments were simply gone. The parent chain cannot give them back either: once the outputs are spent there is nothing left to scan for. Record every destination, and emit one row per destination when reading the history back. The fee belongs to the transaction rather than to any one payment, so it rides on the first row only -- otherwise anyone adding up the column counts it once per recipient. Sends recorded before this keep their single row: the information was never written down, and inventing it would be worse than the gap.
Three costs grew with the number of sends ever made, and one of them grew with its square. - Reading the history re-parsed the entire coin record INSIDE the loop, once for every send whose outputs were already spent -- which, given time, is nearly all of them. A thousand sends meant a thousand reads of one file. Read it once. - Every send was re-checked against the parent chain on every read: up to two calls each, for ever, including transactions buried months ago. Six confirmations is final, so note it and stop asking. The marks are written once per read rather than once per send, or the saving would have moved from the network to the disk. - Rebuilding the record asked the parent chain about every send ever made. Settled ones have nothing left to say. And one that was not about scale at all: a send took up to a minute to appear. The overview re-reads the balance at most once a minute -- a limit that exists so a failing read cannot become a retry storm -- but it was applied to events too, including the users own send. Bitcoin Core shows the same transaction instantly because it holds it. Now a move we have been told about is served at the next tick, and the minute only rate-limits the failures it was written for.
… holds The scenario fabricates 503 recorded sends -- 500 buried, 3 still waiting -- because a wallet with a year behind it is not something a test can produce one transaction at a time, and counts the calls the read costs with getmainchainrpcstats rather than timing it. It found the previous commit to be half a fix. Marking settled sends and skipping them when rebuilding the record saved nothing on the path that mattered: reading the history still asked the parent chain about every send, 503 calls for 503 sends. Two things were missing. - The calls had to stop for settled sends, not just the marking. - To stop asking, the confirmations must be computable: hence the height a send settled at, recorded when it is marked. And a case the test found on its own: a send marked settled WITHOUT a height -- which is what every wallet upgrading from the previous build has -- fell through to asking again. Buried is buried either way. 503 sends now read with 3 calls.
The Bitcoin fee panel was written against a Send dialog that built its fee controls out of a shared FeeSelectionWidget. This base solves the same problem differently -- its own grid, no such widget -- so the member that held it, and the comment describing it, arrived here with nothing behind them, and the header stopped compiling. The panel itself needs none of that: it attaches to ui->frameFee, which this base has, at the point where that frame is hidden because every recipient is being paid in bitcoin.
The rebase onto v24.7.7 brought in a timer this base already had. Both blocks ran in the constructor: the first was allocated and connected, then the second overwrote the pointer, leaving a timer that is connected to refreshTxSize and can never be started -- and leaving the reader to wonder which of the two is the live one. Nothing was visibly wrong, which is the reason to fix it now rather than when someone changes one of the two and cannot work out why it had no effect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Left behind by an assertion that changed shape while the test was being written. flake8 F401, and the lint job is right to stop on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Rebased onto v24.7.7.
The problem
The wallet held no record of its parent-chain coins. Every question about
Bitcoin — what is my balance, what can I spend — was answered by rebuilding the
answer from nothing, with a
scantxoutsetover the entire parent UTXO set.Measured on testnet4, 149,775 blocks and 14.2 million unspent outputs:
That single fact produced everything users hit:
a scan sees confirmed coins only, so it re-offered the coin the first send had
already committed;
that plainly existed.
scantxoutsetis O(current UTXO set), not O(chain since the wallet's birthday):a wallet created this morning pays exactly what a decade-old one pays. On a
mainnet-sized set a single read is minutes, so this is not slow — it does not
work.
What this does
Keeps a record beside the wallet (
parent_coins.json): the coins, the heightand block hash they were current at, and any pending spend of ours. Filled once
by a full scan, then advanced block by block over what arrived since.
Same wallet, same coins, same totals to the satoshi.
and nothing else can see it happen.
that is no longer the chain, everything after it is in doubt and the record is
rebuilt rather than walked forward.
the next send neither collides with the last nor waits for a confirmation.
are stale. A zero balance reads as theft; a dated one reads as weather.
asked about again: 503 recorded sends now read with 3 parent-chain calls.
Also here, because they are the same surface:
getbtcfeerateand a Bitcoin fee panel in Send. The asset fee panel is hiddenfor bitcoin — it prices assets — which left no fee controls at all, while
the parent chain's estimator quoted 362 sat/vB on testnet4: 0.0005 BTC of fee
on a 0.01 BTC send. Recommended now shows the number and Custom lets it be
refused. Bitcoin's
DEFAULT_TRANSACTION_MAXFEEceiling applies to bitcoinsends only.
and costs one fee instead of N. Mixing bitcoin with a Sequentia asset is still
refused — two chains — and now says so while the form is being filled.
Tests
test/functional/feature_parent_coin_record.py: the record is built on firstuse, advanced without sweeping again, sees money arrive, survives a restart, is
rebuilt across a parent reorg, answers stale when the parent is down, and costs
what is in flight rather than what it holds.
Not covered, and said so in the file: sending.
sendbtctoaddressbuilds aBitcoin transaction and the daemon standing in for Bitcoin in the framework is an
Elements one, which cannot decode it. Committing coins to a pending send, banking
its change, and a second immediate send were exercised by hand on testnet4.
Wiring a real
bitcoindparent into the framework should bring those three backhere first.
Known limits
reading the parent mempool, which
getrawmempoolcannot answer cheaply — itreturns ids, not outputs. ZMQ is the road, and it is not this change.
CallMainChainRPCBatch(24.7.5) is the right answer for that path and is not used here yet.
MainchainUnchangedHeightmay be thebetter oracle, with the caveat that the two ask different questions.