This PR adds fuzz coverage for wallet/fees. Some functions may use or not (non default) values from wallet, CCoinControl or FeeCalculation. So the logic is to make the test sometimes fill up some attributes and others no.
Obs: As soon as this PR gets some reviews, I can open the proper PR to qa-assets as well.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Xekyo, MarcoFalke, dergoegge |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
25 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 26 | + CMutableTransaction random_mutable_transaction; 27 | + const auto& node = g_setup->m_node; 28 | + auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())}; 29 | + 30 | + CWallet wallet(node.chain.get(), "", CreateDummyWalletDatabase());
Any reason to create a fresh wallet for each iteration? Should give more iter/second if this was static.
Also, could run clang-format to avoid the leading whitespace bloat?
In fact, there is no need to create it for each iteration, gonna change it. Thanks.
Force-pushed addressing @MarcoFalke's review.
wallet to be staticclang-format25 | +{ 26 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 27 | + CMutableTransaction random_mutable_transaction; 28 | + const auto& node = g_setup->m_node; 29 | + auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())}; 30 | + static CWallet wallet(node.chain.get(), "", CreateDummyWalletDatabase());
This is fine, but I think for documentation purposes, the static globals are initialized in initialize_setup in the fuzz tests
Nice, make sense having it in initialize_setup, if I have to retouch the code again I can do it.
Looks like you forgot to address this?
Yea, sorry!
Seems the way to do it would be:
diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp
index c4e1b7d61..fc9b6a5e4 100644
--- a/src/wallet/test/fuzz/fees.cpp
+++ b/src/wallet/test/fuzz/fees.cpp
@@ -15,23 +15,27 @@
namespace wallet {
namespace {
const TestingSetup* g_setup;
+static std::unique_ptr<CWallet> wallet_ptr;
+static Chainstate* chainstate = nullptr;
void initialize_setup()
{
static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
g_setup = testing_setup.get();
+ const auto& node{g_setup->m_node};
+ chainstate = &node.chainman->ActiveChainstate();
+ wallet_ptr = std::make_unique<CWallet>(node.chain.get(), "", CreateDummyWalletDatabase());
}
FUZZ_TARGET_INIT(wallet_fees, initialize_setup)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
CMutableTransaction random_mutable_transaction;
- const auto& node{g_setup->m_node};
- const auto& chainstate{node.chainman->ActiveChainstate()};
- static CWallet wallet(node.chain.get(), "", CreateDummyWalletDatabase());
+
+ CWallet& wallet = *wallet_ptr;
{
LOCK(wallet.cs_wallet);
- wallet.SetLastBlockProcessed(chainstate.m_chain.Height(), chainstate.m_chain.Tip()->GetBlockHash());
+ wallet.SetLastBlockProcessed(chainstate->m_chain.Height(), chainstate->m_chain.Tip()->GetBlockHash());
}
if (fuzzed_data_provider.ConsumeBool()) {
Seems reasonable? Any alternative?
Yeah, lgtm. Maybe use g_ prefix for the static globals?
Yea, gonna do it!
24 | +FUZZ_TARGET_INIT(wallet_fees, initialize_setup) 25 | +{ 26 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 27 | + CMutableTransaction random_mutable_transaction; 28 | + const auto& node = g_setup->m_node; 29 | + auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
Any reason for the cast?
Since I'm not using anything related to mempool, I don't think so, will remove it.
61 | + (void)GetMinimumFeeRate(wallet, coin_control, &fee_calculation); 62 | + (void)GetMinimumFee(wallet, tx_bytes, coin_control, &fee_calculation); 63 | + } else { 64 | + (void)GetMinimumFeeRate(wallet, coin_control, nullptr); 65 | + (void)GetMinimumFee(wallet, tx_bytes, coin_control, nullptr); 66 | + }
nit for less code (feel free to ignore):
FeeCalculation fee_calculation;
FeeCalculation* maybe_fee_calculation{ fuzzed_data_provider.ConsumeBool()?nullptr:&fee_calculation};
(void)GetMinimumFeeRate(wallet, coin_control, maybe_fee_calculation);
(void)GetMinimumFee(wallet, tx_bytes, coin_control, maybe_fee_calculation);
Seems good, thanks
Force-pushed addressing @MarcoFalke's review.
chainstate - #27647 (review)FeeCalculation has been simplified - #27647 (review)
Force-pushed
initialize_setup - #27647 (review)Thanks @MarcoFalke for the review
CI failure seems unrelated
Force-pushed to replace CreateMockWalletDatabasefor CreateMockableWalletDatabase.
28 | +} 29 | + 30 | +FUZZ_TARGET_INIT(wallet_fees, initialize_setup) 31 | +{ 32 | + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; 33 | + CMutableTransaction random_mutable_transaction;
random_mutable_transaction appears to be of no use.
Removed it, thanks!
Force-pushed removing the unused variable random_mutable_transaction
21 | +void initialize_setup() 22 | +{ 23 | + static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); 24 | + g_setup = testing_setup.get(); 25 | + const auto& node{g_setup->m_node}; 26 | + g_chainstate = &node.chainman->ActiveChainstate();
Any reason to make this a global when it can be just a reference local in the body of FUZZ_TARGET_INIT?
No, going to change it.
0 | @@ -0,0 +1,68 @@ 1 | +// Copyright (c) 2022 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | + 5 | +#include <test/fuzz/FuzzedDataProvider.h> 6 | +#include <test/fuzz/fuzz.h> 7 | +#include <test/fuzz/util.h> 8 | +#include <test/fuzz/util/mempool.h>
not needed?
Yes, I agree that fuzz/util/mempool.h is not required here. I think we need to include validation.h for the ActiveChainstate method.
The reason this does not result in a compilation error is that fuzz/util/mempool.h includes the validation.h file.
lgtm ACK 4da8d6bf17c0875c1a8f60ad2bb1bbd418d3a7cd
left some nits, didn't test
Force-pushed addressing #27647 (review) and #27647 (review)
CI error is unrelated.
ACK 162602b208493e7da1984044e661402c3e52932b
lgtm ACK 162602b208493e7da1984044e661402c3e52932b
Code review ACK 162602b208493e7da1984044e661402c3e52932b
When adding a fuzz target it makes sense to run it with the sanitizers enabled, so that bugs are caught and fixed either before or with adding the target.
echo 'OiAAAPr//wAAAAAAAAA=' | base64 --decode > /tmp/a
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=wallet_fees ./src/test/fuzz/fuzz /tmp/a
wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed)
[#0](/bitcoin-bitcoin/0/) 0x5625ef46a094 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) src/wallet/fees.cpp:58:58
[#1](/bitcoin-bitcoin/1/) 0x5625eedd467f in wallet::(anonymous namespace)::wallet_fees_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/fees.cpp:64:11
...
SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change wallet/fees.cpp:58:58 in
