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.
<!--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 |
|---|---|
| ACK | frankomosh |
| 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:
removeprunedfunds and delete transaction delete code by davidgumberg)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.
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.
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?
Good point, I think the comment should be improved to reflect it - "one index per iteration" might not be correct. Thanks.
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?
No reason, will leave as is but can think about it in case I have to touch it again. Thanks.
Force-pushed addressing #36068 (review)
63 | + // m_iterations * m_spkms.size() as a tripwire: growing faster than that 64 | + // means an iteration is leaving behind state this class does not know 65 | + // about. 66 | + const int64_t drift{RangeTotal() - m_baseline_range_total}; 67 | + assert(drift >= 0); 68 | + assert(static_cast<uint64_t>(drift) <= m_iterations * m_spkms.size());
I think that this check runs at the start of an input, but checks the state left by earlier inputs. So when one input leaks, the crash is reported on the next input, which then does not reproduce on its own.
To test this, I made one input (A) leak 100 change keys, and ran it before a normal input (B):
A then B -> crash reported on B
B alone -> no crash
A alone -> no crash (with -detect_leaks=0)
Maybe we could check at the end of each input instead , so the crash lands on the right input..?..That would also seem to allow for a much tighter bound: on my corpus side with 4640 inputs, no input moved the range by more than 2 (0: 4490, 1: 147, 2: 4).
<details> <summary><strong>A suggestion of a potential fix diff </strong></summary>
--- a/src/test/fuzz/util/wallet.h
+++ b/src/test/fuzz/util/wallet.h
@@ void Reset()
assert(wallet->GetTXOs().empty());
- // Deriving a change address advances one descriptor's next_index and
- // extends its range. A single CreateTransaction() can do this more than
- // once: the avoid-partial-spends retry reserves a second change key on
- // the same descriptor when the first attempt was changeless. Rather
- // than track the exact count, bound the total drift loosely at
- // m_iterations * m_spkms.size() as a tripwire: growing faster than that
- // means an iteration is leaving behind state this class does not know
- // about.
- const int64_t drift{RangeTotal() - m_baseline_range_total};
- assert(drift >= 0);
- assert(static_cast<uint64_t>(drift) <= m_iterations * m_spkms.size());
}
@@
CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) {
return GetScriptForDestination(GetDestination(fuzzed_data_provider));
}
+ int64_t RangeTotal() const
+ {
+ int64_t total{0};
+ for (const DescriptorScriptPubKeyMan* spkm : m_spkms) total += spkm->GetEndRange();
+ return total;
+ }
+
private:
@@
std::vector<DescriptorScriptPubKeyMan*> m_spkms;
- int64_t m_baseline_range_total{0};
uint64_t m_iterations{0};
@@ void Build()
PrecomputeDestinations();
- m_baseline_range_total = RangeTotal();
m_iterations = 0;
}
@@
-
- int64_t RangeTotal() const
- {
- int64_t total{0};
- for (const DescriptorScriptPubKeyMan* spkm : m_spkms) total += spkm->GetEndRange();
- return total;
- }
-
};
--- a/src/wallet/test/fuzz/spend.cpp
+++ b/src/wallet/test/fuzz/spend.cpp
@@
FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
FuzzedWallet& fuzzed_wallet{*g_wallet};
fuzzed_wallet.Reset();
+ // Checked when this input ends, so a failure is reported on the input that caused it.
+ // At most 2: one change key, plus one more if the avoid-partial-spends retry runs.
+ struct DriftCheck {
+ FuzzedWallet& w;
+ int64_t before;
+ ~DriftCheck() { assert(w.RangeTotal() - before <= 2); }
+ } drift_check{fuzzed_wallet, fuzzed_wallet.RangeTotal()};
</details>
Good catch, better to check at the end. I'm going to address your suggestion, but also I'm going to add a non-negative assertion.
Concept ACK on improving performance
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.
Force-pushed addressing #36068 (review)
{
LOCK(fuzzed_wallet.wallet->cs_wallet);
assert(fuzzed_wallet.wallet->GetTXOs().size() == static_cast<size_t>(next_locktime));
}
Asserting inserted output counts here would help catch any future regression in this harness as you had suggested in #35790#pullrequestreview-4782030244, Initially I proposed it be added in #34264 but the author has not been available for a while.
auto res_ct{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)};
if (res_ct) { assert(res_ct->fee >= 0); }
Also, I think we could do an assert check on the result here ?..had also suggested this elswhere
tACK 12d9ed130f9cbe43e2bec7dd6afd44f53fa6a346. This improves performance of this harness. Added 2 additional comments which are not related to the specific goals of the PR but I think important for this harness, for the author's consideration.
