fuzz test fixes
set -DPRODUCTION for simplicity in fuzz tests
set UNDEBUG for simplicity in fuzz tests
update wallet fuzz tests
update test script
fix msvc conversion issues
more msvc vector conversion fixes and substitution of boost libs
fix more hexstr calls
fix hexstr calls in init.cpp
default CMAKE_GENERATOR to Unix Makefiles
more span fixes
specify build bin directory
fix executable locations
fix fuzz tests for c11 and functional test fixes
fix fuzz test executable path
specify legacy wallet for elements functional tests
remove assertion for static initialization order issue
use elements fuzz corpus
print debug log on failure
use heap for blind and asset_blind
test: avoid disk space warning for non-regtest
feature_config_args.py incorrectly assumed that its testnet4 node
would not log a disk space warning.
0683b8ebf3 increased m_assumed_blockchain_size
on testnet4 from 1 to 11 GiB which triggers this bug on more
systems, e.g. a RAM disk.
Prevent the warning by setting -prune for these nodes.
Fix the same issue in feature_signet.py
Github-Pull: #32057
Rebased-From: 20fe41e9e83d510fd467f5a999d55a614b16ef89
Using bypass_limits=true is essentially fuzzing part of a
reorg only, and results in TRUC invariants unable to be
checked. Remove most instances of bypassing limits, leaving
one harness able to do so.
Github-Pull: #33504
Rebased-From: bbe8e9063c15dc230553e0cbf16d603f5ad0e4cf
fae63bf130 fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457 fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab fuzz: Add missing SeedRandomStateForTest(SeedRand::ZEROS) (MarcoFalke)
Pull request description:
This is the first step toward improving fuzz stability and determinism (https://github.com/bitcoin/bitcoin/issues/29018).
A fuzz target using the global test-only PRNG will now abort if the seed is re-used across fuzz inputs.
Also, temporarily add `SeedRandomStateForTest(SeedRand::ZEROS)` to all affected fuzz targets. This may slow down the libfuzzer leak detector, but it will disable itself after some time, or it can be disabled explicitly with `-detect_leaks=0`.
In a follow-up, each affected fuzz target can be stripped of the global random use and a local `RandomMixin` (or similar) can be added instead.
(Can be tested by removing any one of the re-seed calls and observing a fuzz abort)
ACKs for top commit:
hodlinator:
ACK fae63bf130
dergoegge:
utACK fae63bf130
marcofleon:
Tested ACK fae63bf130
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
94ed4fbf8e Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63 doc: update package RBF comment (Greg Sanders)
6e3c4394cf mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c9 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448 [test] package rbf (glozow)
dc21f61c72 [policy] package rbf (Suhas Daftuar)
5da3967815 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e
theStack:
Code-review ACK 94ed4fbf8e
murchandamus:
utACK 94ed4fbf8e
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
09ef322acc [[refactor]] Check CTxMemPool options in constructor (TheCharlatan)
Pull request description:
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
ACKs for top commit:
stickies-v:
re-ACK 09ef322acc . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
achow101:
ACK 09ef322acc
ryanofsky:
Code review ACK 09ef322acc. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/28949
I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.
The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.
ACKs for top commit:
ismaelsadeeq:
Re-ACK 38f70ba6ac👍🏾
glozow:
ACK 38f70ba6ac with some non-blocking nits
murchandamus:
LGTM, code review ACK 38f70ba6ac
Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
