fa1a14a13a fuzz: Reset chainman state in process_message(s) targets (MarcoFalke)
fa9a3de09b fuzz: DisableNextWrite (MarcoFalke)
aeeeeec9f7 fuzz: Reset dirty connman state in process_message(s) targets (MarcoFalke)
fa11eea405 fuzz: Avoid non-determinism in process_message(s) target (PeerMan) (MarcoFalke)
Pull request description:
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
### Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism re-appear.
ACKs for top commit:
marcofleon:
ReACK fa1a14a13a
dergoegge:
reACK fa1a14a13a
Tree-SHA512: 37b5b6dbdde6a39b4f83dc31e92cffb4a62a4b8f5befbf17029d943d0b2fd506f4a0833570dcdbf79a90b42af9caca44e98e838b03213d6bc1c3ecb70a6bb135
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
6135e0553e wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC (Ava Chow)
Pull request description:
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful information for future PRs.
Split from #32489
ACKs for top commit:
rkrux:
ACK 6135e05
Sjors:
ACK 6135e0553e
w0xlt:
ACK 6135e0553e
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.
This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
faa3171ff2 ci: Use optimized Debug build type in test-each-commit (MarcoFalke)
fa21c3401e ci: [doc] reword debug log message (MarcoFalke)
Pull request description:
An optimized debug build is mostly as fast as a release build, because hot loops of heavy debug-only code are rare. So use that setting in the test-each-commit CI, to enable more checks almost "for free".
ACKs for top commit:
Prabhat1308:
re-ACK [`faa3171`](faa3171ff2)
willcl-ark:
crACK faa3171ff2
Tree-SHA512: ca041cf7f79d7abb6f93e17b58b2aea730f3bb9fc51256c1ca1b9f7ce7e7188d18fd99d3754cdbe3f504f4e08d560e72d4b7a75409c214ee2c3771c9a8ba7f96
060695c22a test: Failed load after migrate should restore backup (MarcoFalke)
8a4cfddf23 wallet: Set migrated wallet name only on success (Ava Chow)
Pull request description:
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
ACKs for top commit:
davidgumberg:
ACK 060695c22a
furszy:
ACK 060695c22a
rkrux:
ACK 060695c22a
w0xlt:
reACK 060695c22a
pablomartin4btc:
ACK 060695c22a
Tree-SHA512: f4289e0b3dedef0a3d734c18604f2fd0df0dc65e9641bc342cfa45088d2540a3f6631bbea8bdd394b2631fa83b38e8ac37c83cfc4b53b19dcbd0b820a9beb6e4
b59dc21847 doc: Fix typos in asmap README (nervana21)
Pull request description:
This minor PR fixes some spelling mistakes found while reviewing #33026.
ACKs for top commit:
fanquake:
ACK b59dc21847
Tree-SHA512: e76f7f97c10f3e506d024da0cbf804f4975cf07f31f0dd0abad6fcb97a5fa1032087459dba46de3715f6275d47e2fde4d8db3d38341540110d87fd5669855359
We just need enough transactions to push us above the orphanage limits
and trigger trimming. Reducing the number of transactions cuts the
runtime of this test significantly.
faa3e68411 test: Log KeyboardInterrupt as exception (MarcoFalke)
fa30b34026 test: Do not pass tests on unhandled exceptions (MarcoFalke)
Pull request description:
Currently the functional tests are problematic, because they pass, even if they encounter an unhanded exception.
Fix this by handling all exceptions: Catch `BaseException` as fallback and mark it as failure.
Can be tested via:
```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index da6e5d408f..ecc41fb041 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -19,6 +19,7 @@ class DisableWalletTest (BitcoinTestFramework):
self.wallet_names = []
def run_test (self):
+ import sys;sys.exit("fatal error")
# Make sure wallet is really disabled
assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
```
Previously, the test would pass. With this patch, it would fail.
ACKs for top commit:
enirox001:
Looks good to me—ACK faa3e68
stickies-v:
re-ACK faa3e68411
pablomartin4btc:
tACK faa3e68411
Tree-SHA512: 11ecd5201982e2c776e48d98834b17c15a415306a95524bc702daeba20a316aac797748e9592be8db575597804f149ee7ef104416037cc9e5891758625810e2d
Mark blockhashes and scanobjects arguments as required, so the user receives
a clear help message when either is missing.
Added a new functional test for this use case.
Co-authored-by: stickies-v <stickies-v@users.noreply.github.com>
Add self.log.info(...) calls at the beginning of each test
in GetBlocksActivityTest.
This improves readability and provides debugging information
by logging the purpose of each test upon its correct
execution.
This is in preparation for the next commit, which adds a new test
with log info, and it would look inconsistent without this commit.
The unloadwallet RPC previously failed with a low-level JSON parsing error
when called without any arguments (wallet_name).
Although this issue was first identified during review of the original unloadwallet
implementation in #13111, it was never addressed.
Raise RPC_INVALID_PARAMETER instead describing that either the RPC endpoint or wallet
name must be provided.
Adding a new functional test for this use case.
Refactor migratewallet to use the same logic as the wallet_name argument handling
is identical.
Co-authored-by: maflcko <maflcko@users.noreply.github.com>
2dfeb6668c wallet: remove outdated `pszSkip` arg of database `Rewrite` func (rkrux)
Pull request description:
This argument might have been used in the legacy wallets, but I don't see any implementation using this argument in the SQLite wallets. Removing it cleans up the code a bit.
ACKs for top commit:
achow101:
ACK 2dfeb6668c
brunoerg:
code review ACK 2dfeb6668c
Tree-SHA512: de2178ad6862125f084434ec6a7271d567544870c474c5ea2e75a4f69f3f5eb2170ff46947e098f58e1fa47c35bbe4ebafcd8180581d1f100f1f8d177b32dd91
06ab3a394a tests: speed up coins_tests by parallelizing (Anthony Towns)
Pull request description:
Updates the cmake logic to generate a separate test for each BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp into three separate suites so that they can be run in parallel. Also updates the convention enforced by test/lint/lint-tests.py.
ACKs for top commit:
l0rinc:
reACK 06ab3a394a
maflcko:
lgtm ACK 06ab3a394a
achow101:
ACK 06ab3a394a
Tree-SHA512: 940d9aa31dab60d1000b5f57d8dc4b2c5b4045c7e5c979ac407aba39f2285d53bc00c5e4d7bf2247551fd7e1c8681144e11fc8c005a874282c4c59bd362fb467
065e42976a test: IsFinalTx returns true when there is no locktime (brunoerg)
Pull request description:
According to https://corecheck.dev/mutation/src/consensus/tx_verify.cpp, there is no proper test for the `tx.nLockTime == 0` check in the `IsFinalTx` function, which is understandable, since this check will only be useful for a specific case where the `nBlockHeight` (block height) is zero. Otherwise, the following check `if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))` would catch any of it. This PR adds a test case for it.
ACKs for top commit:
maflcko:
lgtm ACK 065e42976a
enirox001:
ACK 065e429: Valuable test case that explicitly demonstrates `IsFinalTx` behavior when nLockTime is 0
achow101:
ACK 065e42976a
darosior:
utACK 065e42976a
Tree-SHA512: e44a7c060bd4c3d746fab166442cadc3fd449ddd8b02cabf22024a5dde6f438f24c6e1bff2a6dc49b57c8e01234aa0fd393fbfe6194df9d9b6c3d4fa2655c99b
Add a new function called EnsureUniqueWalletNamet that returns the
selected wallet name across the RPC request endpoint and wallet_name.
Supports the case where the wallet_name argument may be omitted—either
when using a wallet endpoint, or when not provided at all. In the latter
case, if no wallet endpoint is used, an error is raised.
Internally reuses the existing implementation to avoid redundant URL
decoding and logic duplication.
This is a preparatory change for upcoming refactoring of unloadwallet
and migratewallet, which will adopt EnsureUniqueWalletName for improved
clarity and consistency.
Better reflect in the documentation that the two methods should be
used in different contexts.
Also update the outdated "call the function without a parameter" phrasing
in the cached version. This wording was accurate when the cache was
introduced in #18991, but became outdated after later commits
(f26502e9fc,
81b00f8780) added parameters to each
function, and the previous commit changed the function naming completely.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
Updates the cmake logic to generate a separate test for each
BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp
into three separate suites so that they can be run in parallel. Also
updates the convention enforced by test/lint/lint-tests.py.
249889bee6 orphanage: avoid vtx iteration when no orphans (furszy)
41ad2be434 mempool: Avoid expensive loop in `removeForBlock` during IBD (Lőrinc)
Pull request description:
During Initial Block Download, the mempool is usually empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with `40 bytes * vtx.size()`, even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this change introduces a minor performance & memory optimization by only executing the loop if any of the affected mempool maps have any contents. The second commit is cherry-picked from there since it's related to this change as well.
ACKs for top commit:
optout21:
ACK 249889bee6
glozow:
ACK 249889bee6
ismaelsadeeq:
reACK 249889bee6
Tree-SHA512: 80d06ff1515164529cdc3ad21db3041bb5b2a1d4b72ba9e6884cdf40c5f1477fee7479944b8bca32a6f0bf27c4e5501fccd085f6041a2dbb101438629cfb9e4b
31c4e77a25 test: fix ReadTopologicalSet unsigned integer overflow (ismaelsadeeq)
Pull request description:
This PR is a simple fix for a potential unsigned integer overflow in ReadTopologicalSet.
We obtain the value of `mask` from fuzz input, which can be the maximum representable value.
Adding 1 to it would then cause an overflow.
The fix skips the addition when the read value is already the maximum.
See https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569 for more context
ACKs for top commit:
maflcko:
lgtm ACK 31c4e77a25
Tree-SHA512: f58d7907f66a0de0ed8d4b1cad6a4971f65925a99f3c030537c21c4d84126b643257c65865242caf7d445b9cbb7a71a1816a9f870ab7520625c4c16cd41979cb
fad040a578 ci: Use APT_LLVM_V in msan task (MarcoFalke)
Pull request description:
This skips compilation of clang by using the apt.
ACKs for top commit:
m3dwards:
ACK fad040a578
willcl-ark:
ACK fad040a578
Tree-SHA512: cc8977a0e97f731b15a2bb9321442d4fc935e310a9cd1993c4ec08ddfd8d7f08a128bbe51ad4d820627bbdcdc748dd58feeec00dee6ee0723e528c546d209f92
