FuzzedWallet was rebuilt for every input. That imports eight descriptors, and each address it then hands out costs a BIP32 derivation plus three separate WalletBatch transactions (see DescriptorScriptPubKeyMan::GetNewDestination) - all paid before the target reaches the code under test.
This PR changes it to build the wallet once in the target's .init hook and reset it perinput instead. I benchmarked on my Ubuntu machine without sanitizers and got a ~6x speedup.
Add CWallet::ClearInMemoryTxStateForTest(), which drops mapWallet,
wtxOrdered, mapTxSpends, m_txos and m_locked_coins, leaving the
database, the wallet flags and the ScriptPubKeyMans untouched.
This exists so that fuzz harnesses can reuse a single wallet across
inputs instead of rebuilding it for every one. m_txos and mapTxSpends
are private, so a harness cannot reset them on its own.
RemoveTxs() is not usable here: it erases each transaction's
m_it_wtxOrdered from wtxOrdered, and that iterator is only valid for
transactions added through AddToWallet(). Harnesses that populate
mapWallet directly leave it uninitialized.
FuzzedWallet was rebuilt for every input. That imports eight
descriptors, and each address it then hands out costs a BIP32
derivation plus three separate WalletBatch transactions (see
DescriptorScriptPubKeyMan::GetNewDestination) - all paid before the
target reaches the code under test.
Build the wallet once in the target's .init hook and reset it per
input instead.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36068.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Approach ACK | jeanpablojp |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
CAmount a class by hodlinator)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Approach ACK
Ran the qa-assets corpus on master and on this head, the speedup reproduces, around 4x here.
One thing I didn't expect. With the fixed pool the biggest OutputGroup in this target goes from 1 to 19, so GroupOutputs finally has something to group. I didn't see the pool mentioned in the PR description or the commits, might be worth a line?
Left two inline comments.
49 | + } 50 | + m_cursors.fill(0); 51 | + m_change_cursors.fill(0); 52 | + { 53 | + LOCK(wallet->cs_wallet); 54 | + wallet->ClearInMemoryTxStateForTest();
The descriptor's next_index isn't reset here, so the same input builds a different transaction depending on how many ran before it. I don't think it hurts in practice since only the change output and its signature move, but the comment on the drift assert assumes an iteration advances one index and the APS retry can keep two. Is this intended?
2492 | @@ -2493,6 +2493,17 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs 2493 | return {}; 2494 | } 2495 | 2496 | +void CWallet::ClearInMemoryTxStateForTest() 2497 | +{ 2498 | + AssertLockHeld(cs_wallet); 2499 | + mapWallet.clear();
Nit, mapWallet.clear() runs before wtxOrdered and m_txos, and both point into it. Nothing dereferences in between so it's latent, but RemoveTxs does it the other way round and leaves mapWallet for last. Any reason for the order?
