ee1128ead846698db5e5633f193883837f2fbc64 doc: update stack-clash-protection comment re mingw-w64 (fanquake)
bf47448f152316145d9abb9b8abc3b564194fe46 test: drop check for Windows < 10 (fanquake)
35b898c47f8af6807c4a5f404af165c663c81a99 release: target Windows 10 or later (fanquake)
398754e70bc96b86ad0327fbe70fafdf27bb4e35 depends: target Windows 10 when building for mingw-w64 (fanquake)
Pull request description:
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.
We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.
Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
ACKs for top commit:
hodlinator:
cr-ACK ee1128ead846698db5e5633f193883837f2fbc64
davidgumberg:
ACK ee1128ead8
achow101:
ACK ee1128ead846698db5e5633f193883837f2fbc64
hebasto:
re-ACK ee1128ead846698db5e5633f193883837f2fbc64, only rebased, a commit message and a comment have been amended since my recent [review](https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160).
TheCharlatan:
ACK ee1128ead846698db5e5633f193883837f2fbc64
Tree-SHA512: 245e0bac3d63414d919a1948661fef4ff79359faaacaf19d64abd91cc62e822797fb1cf3379e340bfdf9a85c0b88fd99a90eda450dd4218b6213ab78aefb1374
11f3bc229ccd4b20191855fb1df882cfa6145264 refactor: Reserve vectors in fuzz tests (Lőrinc)
152fefe7a22b7da3cfe2815083634bece9c5654e refactor: Preallocate PrevectorFillVector(In)Direct without vector resize (Lőrinc)
a774c7a339c26b1409c9a9572d2b52810ee64062 refactor: Fix remaining clang-tidy performance-inefficient-vector errors (Lőrinc)
Pull request description:
PR inspired by https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307 (and https://github.com/bitcoin/bitcoin/pull/29458, https://github.com/bitcoin/bitcoin/pull/29606, https://github.com/bitcoin/bitcoin/pull/29607, https://github.com/bitcoin/bitcoin/pull/30093).
The `clang-tidy` check can be run via:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-inefficient-vector-operation' | grep -v 'clang-tidy'
```
which revealed 3 tests and 1 prod warning (+ fuzz and benching, found by hebasto).
Even though the tests aren't performance critical, getting rid of these warnings (for which the checks were already enabled via https://github.com/bitcoin/bitcoin/blob/master/src/.clang-tidy#L18, see below), the fix was quite simple.
<details>
<summary>clang-tidy -list-checks</summary>
```bash
cd src && clang-tidy -list-checks | grep 'vector'
performance-inefficient-vector-operation
```
</details>
<details>
<summary>Output before the change</summary>
```
src/test/rpc_tests.cpp:434:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
433 | for (int64_t i = 0; i < 100; i++) {
434 | feerates.emplace_back(1 ,1);
| ^
src/test/checkqueue_tests.cpp:366:13: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
365 | for (size_t i = 0; i < 3; ++i) {
366 | tg.emplace_back(
| ^
src/test/cuckoocache_tests.cpp:231:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
228 | for (uint32_t x = 0; x < 3; ++x)
229 | /** Each thread is emplaced with x copy-by-value
230 | */
231 | threads.emplace_back([&, x] {
| ^
src/rpc/output_script.cpp:127:17: error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
126 | for (unsigned int i = 0; i < keys.size(); ++i) {
127 | pubkeys.push_back(HexToPubKey(keys[i].get_str()));
| ^
```
And the fuzz and benchmarks, noticed by hebasto: https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483124499
</details>
ACKs for top commit:
maflcko:
review ACK 11f3bc229ccd4b20191855fb1df882cfa6145264 🎦
achow101:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
theuni:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
hebasto:
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264, tested with clang 19.1.5 + clang-tidy.
Tree-SHA512: 41691c19f35c63b922a95407617a54f9bff1af3f95f99d15642064f321df038aeb1ae5f061f854ed913f69036807cc28fa6222b2ff4c24ef43b909027fa0f9b3
c288c790cd9abe91e53164aba5d975ef1e26ee3f interpreter: Use the same type for SignatureHash in the definition (TheCharlatan)
Pull request description:
This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with:
```
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
```
With this patch it is possible to link against the static consensus library and produce a fully static executable.
ACKs for top commit:
l0rinc:
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
maflcko:
review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f 🐺
achow101:
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
theuni:
Obvious fix ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f.
BrandonOdiwuor:
Code Review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f
Tree-SHA512: 74f283637f0a9cd0cab65d3502f2f8fc4fb983c7672f24e7a76ba2eb6e53b4a81cca0aacb610ef39ac0a454305be594ab440a697ae3718987bf5dbcbc7146a31
b031b7910d62819443813b91705211c466933a53 [ci] Split out native fuzz jobs for macOS and windows (dergoegge)
Pull request description:
Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.
In both jobs the fuzz binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.
ACKs for top commit:
maflcko:
re-lgtm ACK b031b7910d62819443813b91705211c466933a53
achow101:
ACK b031b7910d62819443813b91705211c466933a53
hebasto:
ACK b031b7910d62819443813b91705211c466933a53.
Tree-SHA512: 7d0dc5a9cb299f6f4596dd9a5526b6aaf82efc6eba308bdc9d8b0a45f79dea87204fb6cd4b2ea2a1bd952466b2e958d64021999296d110d7a83c1667f4de51fe
* Since the main LIMITED_WHILE stated `outpoints.size() < 200'000`, I've presized outpoints accordingly.
* `tx_mut.vin` and `tx_mut.vout` weren't caught by the clang-tidy, but addressed them anyway.
This was missed during the original PR switching the nHashType argument
to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
The problem was discovered after running into a linker error when
attempting to link this code as a static library using the header as a
declaration with a riscv32 bare metal toolchain. The compiler would
error with:
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
a0eafc10f94362408f54195ffd5a9237dc1ef638 functional test: Deduplicate assert_mempool_contents() (Hodlinator)
Pull request description:
Recently added `mempool_util` implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba4b9bdbcabaf5b4346610922f0728bb (related comments: https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825278134, https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2445579323).
ACKs for top commit:
instagibbs:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
achow101:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
l0rinc:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
theStack:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
Tree-SHA512: 25ea807d7c041c18be0e4f424131419365d7c1e0fc6c4fb7ac7289c2f8196fd341ff2a2a3ea88df2c3a389edb4571a5fb889efc1b0204c65f7e09ef8f608d0d3
92d3d691f097ead8e5ae571eb9bf691133a6aa49 fuzz: Implement G_TEST_GET_FULL_NAME (Hodlinator)
Pull request description:
Catching up to bench & unit tests. Makes for more orderly paths for fuzz tests using `BasicTestingSetup`.
### Before
```
/tmp/test_common bitcoin/0748ae43ef8fa80703bc/regtest/blocks/xor.dat
```
### After
```
/tmp/test_common bitcoin/tx_pool_standard/f18b3744625e0600eb0c/regtest/blocks/xor.dat
```
ACKs for top commit:
kevkevinpal:
ACK [92d3d69](92d3d691f0)
furszy:
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
tdb3:
ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
dergoegge:
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
brunoerg:
code review ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
Tree-SHA512: 5e83970b111232adece10f79e3a43d0c3c49ab635763e2a4b420f1336cbb8fee94aab751264ddec01ac8363166636e3b29cfe3b2969fc28c8dd6b31bda351950
fe3457ccfff9a022d9f183e18217422e2e1f7689 ci: note that we should install pkgconf in future (fanquake)
8d203480b338c7504887777a8857e91708deadc7 doc: migrate from pkg-config to pkgconf in macOS build docs (fanquake)
Pull request description:
Migrate the macOS build docs and CI from `pkg-config` to `pkgconf`. As the former now just redirects to the later.
Upstream is currently mass-migrating its formula. i.e https://github.com/Homebrew/homebrew-core/pull/198317.
Fixes#31334.
ACKs for top commit:
maflcko:
ACK fe3457ccfff9a022d9f183e18217422e2e1f7689 🍭
hebasto:
re-ACK fe3457ccfff9a022d9f183e18217422e2e1f7689.
Tree-SHA512: 6e337acb6767d163491149b6ae7181d7d7042bc11cdc745eb6f52d4df6d7a19c4f6daa000b314acd9178f97e670aba145f829e48b1b3033117d7e39cdd3af177
Recently added mempool_util implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba4b9bdbcabaf5b4346610922f0728bb.
Brew has migrated to using the later:
```bash
brew info pkg-config
==> pkgconf: stable 2.3.0 (bottled), HEAD
Package compiler and linker metadata toolkit
https://github.com/pkgconf/pkgconf
```
9aa50152c1cfa1c41215b2b51ed7a516aa67137a Add destroy to BlockTemplate schema (Sjors Provoost)
Pull request description:
This ensures that if a client no longer needs a block template, the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer in the mempool, so this can be significant.
This has a trivial silent merge conflict with #31283 because it also used the number `@9` (gaps are not allowed). I'll rebase whichever is merged last.
ACKs for top commit:
TheCharlatan:
Re-ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
ryanofsky:
Code review ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
Tree-SHA512: 393571b4455969cba71c7572feaeff4503738205331ab198b9181c156c75eb65933a8e5ceff66fc06d1efb8f2528bdb254e5eee7df75735b912526de1e7b166d
When BasicTestingSetup is used in fuzz-tests it will now create test directories containing the fuzz target names. Example:
/tmp/test_common bitcoin/tx_package_eval/153d7906294f7d0606a7/
This is already implemented for bench and unit tests.
5736d1ddacc4019101e7a5170dd25efbc63b622a tracing: pass if replaced by tx/pkg to tracepoint (0xb10c)
a4ec07f1944999c2eead41d08d7dd4fc3aa71243 doc: add comments for CTxMemPool::ChangeSet (Suhas Daftuar)
83f814b1d1100baac9dca9c176f89b0ec2555dbc Remove m_all_conflicts from SubPackageState (Suhas Daftuar)
d3c8e7dfb63f7986a1f9654ea2393aabe3cd78da Ensure that we don't add duplicate transactions in rbf fuzz tests (Suhas Daftuar)
d7dc9fd2f7bc675256687b9c55fdbec9cc8ac781 Move CalculateChunksForRBF() to the mempool changeset (Suhas Daftuar)
284a1d33f1dcbc3b3404ea40a948ff6600239613 Move prioritisation into changeset (Suhas Daftuar)
446b08b599bc492bbec10ccc2292aee6f90c58e7 Don't distinguish between direct conflicts and all conflicts when doing cluster-size-2-rbf checks (Suhas Daftuar)
b53041021abc4f9ee7203341413e8676e2d5a7ca Duplicate transactions are not permitted within a changeset (Suhas Daftuar)
b447416fddcb8c8647391502cca3dbfd1552e02e Public mempool removal methods Assume() no changeset is outstanding (Suhas Daftuar)
2b30f4d36c86f775ac637b171d27d42a02309c5b Make RemoveStaged() private (Suhas Daftuar)
18829194ca68152ac0b38d34e94b9265ee74c410 Enforce that there is only one changeset at a time (Suhas Daftuar)
7fb62f7db60c7d793828ae45f87bc3f5c63cc989 Apply mempool changeset transactions directly into the mempool (Suhas Daftuar)
34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Clean up FinalizeSubpackage to avoid workspace-specific information (Suhas Daftuar)
57983b8add72a04721d3f2050c063a3c4d8683ed Move LimitMempoolSize to take place outside FinalizeSubpackage (Suhas Daftuar)
01e145b9758f1df14a7ea18058ba9577bf88e459 Move changeset from workspace to subpackage (Suhas Daftuar)
802214c0832de00f24268183f7763fa984ba7903 Introduce mempool changesets (Suhas Daftuar)
87d92fa340195d9c87be3d023ca133b90b3b7d4e test: Add unit test coverage of package rbf + prioritisetransaction (Suhas Daftuar)
15d982f91e6b0f145c9dd4edf29827cfabb37a3f Add package hash to package-rbf log message (Suhas Daftuar)
Pull request description:
part of cluster mempool: #30289
It became clear while working on cluster mempool that it would be helpful for transaction validation if we could consider a full set of proposed changes to the mempool -- consisting of a set of transactions to add, and a set of transactions (ie conflicts) to simultaneously remove -- and perform calculations on what the mempool would look like if the proposed changes were to be applied. Two specific examples of where we'd like to do this:
- Determining if ancestor/descendant/TRUC limits would be violated (in the future, cluster limits) if either a single transaction or a package of transactions were to be accepted
- Determining if an RBF would make the mempool "better", however that idea is defined, both in the single transaction and package of transaction cases
In preparation for cluster mempool, I have pulled this reworking of the mempool interface out of #28676 so it can be reviewed on its own. I have not re-implemented ancestor/descendant limits to be run through the changeset, since with cluster mempool those limits will be going away, so this seems like wasted effort. However, I have rebased #28676 on top of this branch so reviewers can see what the new mempool interface could look like in the cluster mempool setting.
There are some minor behavior changes here, which I believe are inconsequential:
- In the package validation setting, transactions would be added to the mempool before the `ConsensusScriptChecks()` are run. In theory, `ConsensusScriptChecks()` should always pass if the `PolicyScriptChecks()` have passed and it's just a belt-and-suspenders for us, but if somehow they were to diverge then there could be some small behavior change from adding transactions and then removing them, versus never adding them at all.
- The error reporting on `CheckConflictTopology()` has slightly changed due to no longer distinguishing between direct conflicts and indirect conflicts. I believe this should be entirely inconsequential because there shouldn't be a logical difference between those two ideas from the perspective of this function, but I did have to update some error strings in some tests.
- Because, in a package setting, RBFs now happen as part of the entire package being accepted, the logging has changed slightly because we do not know which transaction specifically evicted a given removed transaction.
- Specifically, the "package hash" is now used to reference the set of transactions that are being accepted, rather than any single txid. The log message relating to package RBF that happen in the `TXPACKAGES` category has been updated as well to include the package hash, so that it's possible to see which specific set of transactions are being referenced by that package hash.
- Relatedly, the tracepoint logging in the package rbf case has been updated as well to reference the package hash, rather than a transaction hash.
ACKs for top commit:
naumenkogs:
ACK 5736d1ddac
instagibbs:
ACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
ismaelsadeeq:
reACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
glozow:
ACK 5736d1ddacc
Tree-SHA512: 21810872e082920d337c89ac406085aa71c5f8e5151ab07aedf41e6601f60a909b22fbf462ef3b735d5d5881e9b76142c53957158e674dd5dfe6f6aabbdf630b
a6ca8f324396522e9748c9a7bbefb3bf1c74a436 fuzz: Fix difficulty target generation in p2p_headers_presync (marcofleon)
fa327c77e34e0cfb7994842c23f539ab11bf5d3b util: Add ConsumeArithUInt256InRange fuzzing helper (marcofleon)
Pull request description:
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
Input that triggered the failure: [p2ppresync_crash.txt](https://github.com/user-attachments/files/17620203/p2ppresync_crash.txt)
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeArithUInt256InRange` helper function and use it in the fuzz test to generate target values within the originally intended range. The target is then converted to an `nBits` value using `GetCompact()`.
For some more context, see https://github.com/bitcoin/bitcoin/pull/30918.
[setcompact-link]: 6463117a29/src/arith_uint256.h (L251-L271)
ACKs for top commit:
instagibbs:
ACK a6ca8f3243
dergoegge:
Code review ACK a6ca8f324396522e9748c9a7bbefb3bf1c74a436
brunoerg:
code review ACK a6ca8f324396522e9748c9a7bbefb3bf1c74a436
Tree-SHA512: 92013d9d37bd3f11992ee678ba9745196efbdc4d773fd14994116629260bea46ffc9fa3923d443af7b623d39c6211900ce98a349c62ad1976e12312c37ef9df0
637f437a1642ce845d7f750b45b71320a82d83b5 doc: remove PR Review Club frequency (Gabriele Bocchi)
Pull request description:
The PR Review Club is mentioned as weekly in the CONTRIBUTING.md file, but it is held monthly as per the official [Bitcoin Core PR Review Club website](https://bitcoincore.reviews/). This PR updates the documentation to just remove the frequency.
ACKs for top commit:
fanquake:
ACK 637f437a1642ce845d7f750b45b71320a82d83b5
Tree-SHA512: 27bf8a0e32edd8bedb5301ceb3c744ff4629403292a7ad00b633921f36278443ae297cd53708a533b1d6e6eab863b831e11247b95277b94cce28e3d5ddb7d9b9
