This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if the child bitcoin-wallet process is killed (either by an
external signal or by Ctrl-C as reported in the issue) the bitcoin-node process
will not shutdown cleanly after that because chain client flush()
calls will fail.
This change fixes the problem by handling ipc::Exception errors thrown during
the flush() calls, and it relies on the fixes to disconnect detection
implemented in https://github.com/bitcoin-core/libmultiprocess/pull/160 to work
effectively.
This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is
handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the
wallet to shutdown abruptly instead of waiting for the node and shutting down
cleanly.
This change fixes the problem by having the wallet process print to stdout when
it receives a Ctrl-C signal but not otherwise react, letting the node shut
everything down cleanly.
Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.
Use EventLoopRef to avoid reference counting bugs and be more exception safe
and deal with removal of addClient/removeClient methods in
https://github.com/bitcoin-core/libmultiprocess/pull/160
A test update is also required due to
https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed
reference count semantics. In IpcPipeTest(), it is is now necessary to destroy
the client Proxy object instead of just the client Connection object to
decrease the event loop reference count and allow the loop to exit so the test
does not hang on shutdown.
facb9b327b scripted-diff: Use bpf_cflags (MarcoFalke)
fa0c1baaf8 test: Add imports for util bpf_cflags (MarcoFalke)
Pull request description:
On some Linux kernel versions, the bpf compiler invoked in the functional tests will issue a `-Wduplicate-decl-specifier` warning.
This seems harmless and should be fixed upstream in the Linux kernel.
Here, simply suppress it for now. Fixes https://github.com/bitcoin/bitcoin/issues/32322
ACKs for top commit:
laanwj:
Code review ACK facb9b327b
hebasto:
ACK facb9b327b, I have reviewed the code and it looks OK.
Tree-SHA512: 53387127e3c2a2dbfe05281b2d2e61efbd3c3adcc3b4bf2f11540042f86e1e8c06637f80d246310bc44ca0612318472f25545c1e1ca3636fda97d04381f9e905
3c3548a70e validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock (Matt Corallo)
aac5488909 validation: correctly update BlockStatus for invalid block descendants (stratospher)
9e29653b42 test: check BlockStatus when InvalidateBlock is used (stratospher)
c99667583d validation: fix traversal condition to mark BLOCK_FAILED_CHILD (stratospher)
Pull request description:
This PR addresses 3 issues related to how `BLOCK_FAILED_CHILD` is set:
1. In `InvalidateBlock()`
- Previously, `BLOCK_FAILED_CHILD` was not being set when it should have been.
- This was due to an incorrect traversal condition, which is fixed in this PR.
2. In `SetBlockFailure()`
- `BLOCK_FAILED_VALID` is now cleared before setting `BLOCK_FAILED_CHILD`.
3. In `InvalidateBlock()`
- if block is already marked as `BLOCK_FAILED_CHILD`, don't mark it as `BLOCK_FAILED_VALID` again.
Also adds a unit test to check `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` status in `InvalidateBlock()`.
<details>
<summary><h3>looking for feedback on an alternate approach</h3></summary>
<br>
An alternate approach could be removing `BLOCK_FAILED_CHILD` since even though we have a distinction between
`BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase, we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them. See similar discussion in https://github.com/bitcoin/bitcoin/pull/16856.
I have a branch with this approach in https://github.com/stratospher/bitcoin/commits/2025_02_remove_block_failed_child/.
Compared to the version in #16856, it also resets `BLOCK_FAILED_CHILD` already on disk to `BLOCK_FAILED_VALID` when loading from disk so that we won't be in a dirty state in a no-`BLOCK_FAILED_CHILD`-world.
I'm not sure if it's a good idea to remove `BLOCK_FAILED_CHILD` though. would be curious to hear what others think of this approach.
thanks @ mzumsande for helpful discussion regarding this PR!
</details>
ACKs for top commit:
achow101:
ACK 3c3548a70e
TheCharlatan:
Re-ACK 3c3548a70e
mzumsande:
re-ACK 3c3548a70e
Tree-SHA512: 83e0d29dea95b97519d4868135c965b86f6f43be50b15c0bd8f998b3476388fc7cc22b49c0c54ec532ae8222e57dfc436438f0c8e98f54757b384f220488b6a6
55b931934a removed duplicate calling of GetDescriptorScriptPubKeyMan (Saikiran)
Pull request description:
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs https://github.com/bitcoin/bitcoin/issues/32013.
**Steps to reproduce in testnet environment**
**Input size:** 2 million address in the wallet
**Step1:** call importaddresdescriptor rpc method
observe the time it has taken.
**With the provided fix:**
Do the same steps again
observe the time it has taken.
There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)
main changes i've made during this pr:
1. remove duplicate call to GetDescriptorScriptPubKeyMan method
2. And inside GetDescriptorScriptPubKeyMan method previously we checking **each address linearly** so each time it is calling HasWallet method which has aquired lock.
3. Now i've modified this logic call **find method on the map (O(logn)**) time it is taking, so only once we calling HasWallet method.
**Note:** Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.
ACKs for top commit:
achow101:
ACK 55b931934a
w0xlt:
ACK 55b931934a
Tree-SHA512: 4a7fdbcbb4e55bd034e9cf28ab4e7ee3fb1745fc8847adb388c98a19c952a1fb66d7b54f0f28b4c2a75a42473923742b4a99fb26771577183a98e0bcbf87a8ca
faca46b042 test: Run all benchmarks in the sanity check (MarcoFalke)
Pull request description:
It is unclear why not all benchmarks are run, given that:
* they only run as a sanity check (fastest version)
* no one otherwise runs them, not even CI
* issues have been missed due to this
ACKs for top commit:
l0rinc:
ACK faca46b042
BrandonOdiwuor:
Code Review ACK faca46b042
Tree-SHA512: 866f1ccff0313017dd313d5a218d7ee088b823601a129b9ed4c5819b0d57fd808d78e3ea28ca00714ae6b209df5312b7b9dea091b2b028821ff46b8ba263c48a
3669ecd4cc doc: Document fuzz build options (Anthony Towns)
c1d01f59ac fuzz: enable running fuzz test cases in Debug mode (Anthony Towns)
Pull request description:
When building with
BUILD_FOR_FUZZING=OFF
BUILD_FUZZ_BINARY=ON
CMAKE_BUILD_TYPE=Debug
allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug build.
In Debug builds, deterministic fuzz behaviour is controlled via a runtime variable, which is normally false, but set to true automatically in the fuzz binary, unless the FUZZ_NONDETERMINISM environment variable is set.
ACKs for top commit:
maflcko:
re-ACK 3669ecd4cc🏉
marcofleon:
re ACK 3669ecd4cc
ryanofsky:
Code review ACK 3669ecd4cc with just variable renamed and documentation added since last review
Tree-SHA512: 5da5736462f98437d0aa1bd01aeacb9d46a9cc446a748080291067f7a27854c89f560f3a6481b760b9a0ea15a8d3ad90cd329ee2a008e5e347a101ed2516449e
cad39f86fb bench: ensure wallet migration benchmark runs exactly once (Lőrinc)
c1f458aaa0 ci: re-enable all benchmark runs (Lőrinc)
1da11dbc44 bench: clean up migrated descriptor wallets via loader teardown (Lőrinc)
Pull request description:
The low-priority `WalletMigration` benchmark existed for some time but was never run automatically in our CI.
Although the failure first surfaced on Windows as a hang during temporary directory cleanup, it could also be reproduced on Linux and macOS when forcing multiple iterations (e.g. via a long `--min-time`).
### Root causes
1. **Leaked open wallets on Windows**
`MigrateLegacyToDescriptor` produces two new descriptor wallets (the primary spendable wallet and a companion watch‑only wallet). Without unloading them, their database files remained open in the `WalletContext`, blocking directory removal and hanging the test harness.
<details><summary>Details</summary>
```bash
what(): filesystem error: cannot remove all: The process cannot access the file because it is being used by another process [C:\Users\RUNNER\~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\d8ffd89a7700ce01c31f] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\d8ffd89a7700ce01c31f\regtest\wallet.dat]
```
</details>
2. **Undefined behavior on repeated runs**
The benchmark body calls `std::move(wallet)`, invalidating the local `wallet` pointer. Running more than one iteration causes a use-after-move by the sanitizers.
<details><summary>Details</summary>
```bash
error: bench_bitcoin 0x00067927: DW_TAG_member '_M_local_buf' refers to type 0x00000000000b3ba7 which extends beyond the bounds of 0x0006791d
* thread #1, name = 'b-test', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0xc8)
* frame #0: 0x00005555556a3f33 bench_bitcoin`... basic_string<char>::length(this=<unavailable>) const at basic_string.h:1079:16
```
</details>
### Fixes
- **Automatic wallet teardown**
Wrap the benchmark in a `MakeWalletLoader` (owning a `WalletContext`), so that both migrated wallets are unloaded when the loader goes out of scope, eliminating any lingering open files.
- **Re-enable benchmarks in CI**
Drop the temporary filter in GitHub Actions. The `-sanity-check` run already executes each benchmark once, so `WalletMigration` now runs automatically without hangs or crashes.
- **Single iteration**
Configure the microbenchmark with `.epochs(1).epochIterations(1)`, ensuring the migration code runs exactly once and avoiding use-after-move.
No measurable change in benchmark performance.
ACKs for top commit:
maflcko:
review ACK cad39f86fb🍥
furszy:
utACK cad39f86fb
hebasto:
ACK cad39f86fb, tested on Ubuntu 25.04.
Tree-SHA512: 10343ce7ab9b63ba4f51a7673018215577ea7ec188e41d535a66d69d73b85bca6ba301c33f6920c02f8f7d686c75c65c4a4e9bdafb04b60be85d66aa743cfa20
fa653cb416 ci: Add missing -Wno-error=array-bounds to valgrind fuzz (MarcoFalke)
Pull request description:
Due to an upstream GCC issue, any debug/fuzz build which aborts on failed assumes will print a false positive array-bounds warning in `src/test/fuzz/txgraph.cpp`.
This also affects one CI task.
Fix the CI task by ignoring the error for now.
Fixes https://github.com/bitcoin/bitcoin/issues/32276
ACKs for top commit:
fanquake:
ACK fa653cb416 - checked native fuzz
Tree-SHA512: 0f6c5ec8d96e0bf96cd008e2de5db59e528086a67dcb77f3e59a0d83225d880a59e960d65c5bc8b5ae3de9d5d301bfc7737d95c282aa1bcc740a42561f610ca8
7f5a35cf4b doc: Add deps install notes for multiprocess (TheCharlatan)
Pull request description:
These just mirror the content in src/ipc/libmultiprocess/doc/install.md
ACKs for top commit:
Sjors:
re-ACK 7f5a35cf4b
ryanofsky:
Code review ACK 7f5a35cf4b just dropping dependencies.md update since last review
Tree-SHA512: f9bf4f54542323aa4a0600db874640e575e40355f08515331a27fb139e6e47ee58aa0c6635206f978696e3da7b5aa93efb45b181b02e99e308537fcb90bd6751
e34f12bdd4 ci: Drop no longer necessary `-Wno-error=array-bounds` (Hennadii Stepanov)
Pull request description:
The build log of the "Linux->Windows cross" CI job no longer shows any `-Warray-bounds` compiler warnings. Therefore, there's no need to suppress them with `-Wno-error=array-bounds`.
I likely overlooked this when reviewing https://github.com/bitcoin/bitcoin/pull/29881, as I can run that CI job locally without such warnings even at commit 785649f397.
ACKs for top commit:
TheCharlatan:
ACK e34f12bdd4
Tree-SHA512: ac66160866097538af6f196c0cb22d370427c59c071b0ddcb1a6717e233bbd3dfed4e090d266221c55ae0ddd3d5dffb0ca7ae01582eda07f25fb886a775b6ac5
The migration benchmark crashes if run more than once, because of `std::move(wallet)` and leaves subsequent iterations in an undefined state - avoiding `UndefinedBehaviorSanitizer` null‑dereference error.
`MigrateLegacyToDescriptor` returns both a spendable descriptor wallet and a watch‑only wallet.
If these remain attached, their files stay open and on Windows this can hang CI when removing the test directory.
By constructing them via `MakeWalletLoader` (which owns the `WalletContext`), both wallets are automatically unloaded when the loader is destroyed at the end.
This ensures no lingering handles or resource leaks when running the benchmark on CI with `-sanity-check`.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
When building with
BUILD_FOR_FUZZING=OFF
BUILD_FUZZ_BINARY=ON
CMAKE_BUILD_TYPE=Debug
allow the fuzz binary to execute given test cases (without actual
fuzzing) to make it easier to reproduce fuzz test failures in a more
normal debug build.
In Debug builds, deterministic fuzz behaviour is controlled via a runtime
variable, which is normally false, but set to true automatically in the
fuzz binary, unless the FUZZ_NONDETERMINISM environment variable is set.
fa86190e6e rpc: Allow fullrbf fee bump (MarcoFalke)
Pull request description:
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
ACKs for top commit:
1440000bytes:
reACK fa86190e6e
achow101:
ACK fa86190e6e
w0xlt:
ACK fa86190e6e
rkrux:
reACK fa86190e6e
glozow:
ACK fa86190e6e
Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf
acee5c59e6 descriptors: Have GetPrivKey fill keys directly (Ava Chow)
4b0303197e descriptors: Move FlatSigningProvider pubkey filling to GetPubKey (Ava Chow)
25a3b9b0f5 descriptors: Have GetPubKey fill origins directly (Ava Chow)
6268bde0af descriptor: Remove unused parent_info from BIP32PUbKeyProvider::GetPubKey (Ava Chow)
0ff072caa1 wallet, rpc: Only allow keypool import from single key descriptors (Ava Chow)
Pull request description:
Instead of having `MakeScripts` infer what pubkeys need to go into the output `FlatSigningProvider`, have each of the `PubkeyProviders` that have `GetPubKey` and `GetPrivKey` called fill it directly with relevant keys and origins.
This allows for keys and origins to be added that won't directly appear in the output, which is necessary for `musig()` descriptors.
Split from #29675
ACKs for top commit:
fjahr:
Code review ACK acee5c59e6
theStack:
re-ACK acee5c59e6
rkrux:
ACK acee5c5
Tree-SHA512: c1841359bcb08cdd433122deef96579236928660785f3357a3eb584e47d290cd1c60ebe8f7fba50f178ba45c9a90773124e0f509e36c5a0df97c1a4890e03e5c
e3d7533ac9 test: improves tapscript unit tests (Ethan Heilman)
3e167085ba test: Ensures test fails if witness is not hex (Ethan Heilman)
Pull request description:
This commit creates new test utilities for future Taproot script tests within script_tests.json. The key features of this commit are the addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and `#TAPROOTOUTPUT#`. These tags streamline the test creation process by eliminating the need to manually generate these components outside the test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
This code was originally part of the OP_CAT PR https://github.com/bitcoin/bitcoin/pull/29247 but was pulled out into a separate PR to reduce the rebase treadmill for the OP_CAT PR.
Additionally this PR adds a check to ensure that if the witness data can not be parsed as hex the test fails. Prior to this PR, the test code would fail silently and set the values it couldn't parse as empty stack elements. This fix was suggested by @instagibbs.
## Rationale
While writing JSON script tests (script_tests.json) for https://github.com/bitcoin/bitcoin/pull/29247 we ran into the following problem. The JSON script tests are simple and easy to write for pre-Tapscript scripts, but adding or changing a Tapscript test requires substantial work per test. Consider the following pre-tapscript test:
```
["'aa' 'bb'", "CAT 0x4c 0x02 0xaabb EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "CAT disabled"]
````
whereas a Tapscript test for the same script (annotated with comments for better readability) would look like:
```
[
[
"aa",
"bb",
"7e4c02aabb87", // output script
"c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d", // control block
0.00000001
],
"",
"0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99", // Tapscript output
"P2SH,WITNESS,TAPROOT",
"OK",
"TAPSCRIPT CATs aa and bb together and checks if EQUAL to aabb"
]
```
Computing the Tapscript output, such as `0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99`, requires writing custom code and running it for each test. The same is true for the Tapscript control block, such as `c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d`. If a test is changed or updated new outputs and control blocks must be computed. The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.
In this PR we address this issue by adding the following improvements to JSON script tests:
Adding simple macros ("#SCRIPT# and #CONTROLBLOCK#) that allow the script test parser to automatically generate and inject a valid Tapscript output and control block to be computed automatically from the JSON script.
Allowing Tapscript scripts to use the human readable strings like pre-script scripts by marking the location of the script in the witness stack using #SCRIPT#. This transforms the unreadable script 7e4c02aabb87 into #SCRIPT# CAT 0x4c 0x02 0xaabb EQUAL.
This results in the following JSON script test which is far easier to write and easier to read.
```
[
[
"aa",
"bb",
"#SCRIPT# CAT",
"#CONTROLBLOCK#",
0.00000001
],
"",
"0x51 0x20 #TAPROOTOUTPUT#",
"P2SH,WITNESS,TAPROOT,OP_CAT",
"OK",
"TAPSCRIPT Test of OP_CAT flag by calling CAT on two elements. TAPSCRIPT_OP_CAT flag is set so CAT is executed."
],
```
ACKs for top commit:
instagibbs:
reACK e3d7533ac9
sipa:
utACK e3d7533ac9
janb84:
Re ACK [e3d7533](e3d7533ac9)
Tree-SHA512: 948c3ec28a4b2b222c2d77e48918ed19d298b51d64662fc20959073edd9978fc796516a392da9755a7e173f556e3021816dc6ce8eb3ed16bbe0fa6ebc574fd48
This commit creates new test utilities for future Taproot script
tests within script_tests.json. The key features of this commit are the
addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and
`#TAPROOTOUTPUT#`. These tags streamline the test creation process by
eliminating the need to manually generate these components outside the
test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given
Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
Update src/test/script_tests.cpp
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
18a035145d ci: Temporarily disable `WalletMigration` benchmark (Hennadii Stepanov)
Pull request description:
The `WalletMigration` benchmark is currently failing on CI.
This PR temporarily disables it until the issue is resolved.
An alternative to https://github.com/bitcoin/bitcoin/pull/32302.
ACKs for top commit:
maflcko:
lgtm ACK 18a035145d
TheCharlatan:
ACK 18a035145d
Tree-SHA512: bb1451fd0743a2955216a6d06916e411420a76bfed8b69ffcfadf99d0996d8f3b89ed72f855f25269f943ca4c3b4422065fde2374a1bf76c8bb64f14ab883092
d85895e5a7 build, msvc: Build only required `qtbase` features (Hennadii Stepanov)
fe5a6dcc53 build, msvc: Update vcpkg manifest baseline (Hennadii Stepanov)
Pull request description:
This PR:
1. Updates the vcpkg manifest baseline from the [2024.09.30 Release](https://github.com/microsoft/vcpkg/releases/tag/2024.09.30) to the [2025.03.19 Release](https://github.com/microsoft/vcpkg/releases/tag/2025.03.19), with the following package changes:
- boost: 1.85.0#1,2 --> 1.87.0
- qtbase: 6.7.2#3 -> 6.8.2#1
- qttools: 6.7.2#1 -> 6.8.2
- sqlite3: 3.46.1 --> 3.49.1
The previous update was made in https://github.com/bitcoin/bitcoin/pull/31186.
3. Explicitly specifies required features for the `qtbase` package, which makes vcpkg skip unused features such as `dnslookup`, `openssl`, etc.
ACKs for top commit:
hodlinator:
ACK d85895e5a7
Tree-SHA512: e05fb50825c6ba2e0caf8ded1dfb55fa42f28f06dcd28597f748c1843a1c4cc888e62ad87b6481c66481a576fa224dd4943fe76e286ff579c6d4dc2ec6a78a71
5cb1241814 feefrac: avoid integer overflow in temporary (Pieter Wuille)
Pull request description:
In `FeeFrac::Div(__int128 n, int32_t d, bool round_down)` in src/util/feefrac.h, the following line computes the result:
```c++
return quot + (mod > 0) - (mod && round_down);
```
The function can only be called under conditions where the result is in range, and thus doesn't involve any integer overflow. However, the intermediary result computed by just `quot + (mod > 0)` may still overflow if it's going to be corrected by the `- (mod && round_down)` that follows.
Fix this by balancing the two correction steps with each other first:
```c++
return quot + ((mod > 0) - (mod && round_down));
```
Fixes#32294.
ACKs for top commit:
l0rinc:
Tested ACK 5cb1241814
maflcko:
lgtm ACK 5cb1241814
achow101:
ACK 5cb1241814
Tree-SHA512: 9daaccdf9acd7652d53b52cad2dc12872558265e863acdde2d6015f885cb87c0505f9bd5be5499fc0a0eded29bec719643f6af1fbc3604518143985094226c95
e261eb8d50 tests: Add BIP 373 test vectors (Ava Chow)
26370c68d0 rpc: Include MuSig2 fields in decodepsbt (Ava Chow)
ff3d460898 psbt: Implement un/ser of musig2 fields (Ava Chow)
Pull request description:
Implements un/serialization of MuSig2 PSBT fields and prepares PSBT to be able to sign for MuSig2 inputs.
Split from #29675
ACKs for top commit:
fjahr:
re-ACK e261eb8d50
theStack:
re-ACK e261eb8d50
rkrux:
tACK e261eb8d50
Tree-SHA512: bb852ad074978847ac4dc656332025e2d4d1025d4283537b89618c7cadd61a8ecd2eff24779b8a014bc8d7b431125060449768192fa05ad0577f29e3c64b2374
2835216ec0 txgraph: make GroupClusters use partition numbers directly (optimization) (Pieter Wuille)
c72c8d5d45 txgraph: compare sequence numbers instead of Cluster* (bugfix) (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289
The implicit transaction ordering for transactions in a TxGraphImpl is defined by:
1. higher chunk feerate first
2. lower Cluster* object pointer first
3. lower position within cluster linearization first.
Number (2) is not deterministic, as it intricately depends on the heap allocation algorithm. Fix this by giving each Cluster a unique `uint64_t m_sequence` value, and sorting by those instead.
The second commit then uses this new approach to optimize GroupClusters a bit more, avoiding some repeated checks and dereferences, by making a local copy of the involved sequence numbers.
Thanks to @dergoegge for pointing this out.
ACKs for top commit:
instagibbs:
reACK 2835216ec0
marcofleon:
ACK 2835216ec0
glozow:
utACK 2835216ec0
Tree-SHA512: d772a55b9ed620159b934a42a39fca7f900d4aa89c099a280a0c61ea0bd7c4fc39b388281ffc775064ea77b0b17263871b4c9763aa71c710a79287d5eb2cd4b4
fa6a007b8e fuzz: Avoid integer sanitizer warnings in policy_estimator target (MarcoFalke)
Pull request description:
It seems odd to write a fuzz target to trigger integer sanitizer warnings in `CBlockPolicyEstimator::processBlockTx` and then suppress them. If the scenario can happen in reality, the code should be properly fixed to handle the cases. If not, it seems better to fix the fuzz target to not trigger meaningless traces.
Do that here by keeping track of the current height and limiting mempool entries to at most this entry height.
ACKs for top commit:
brunoerg:
ACK fa6a007b8e
dergoegge:
utACK fa6a007b8e
Tree-SHA512: 2092017dc309fb095fe5d43cfb76efb691795f303d567ee919be2b5cac19a944293636229903dc4d1e8b9fe5daf9dc3058544321eff1735f91f804c3baa36cd0
Also, fix the incorrect documention of the 'replaceable' RPC argument
with respect to sequence number handling. The docs were incorrect
before, so the fix could be extracted, but it seems fine to include here
as well.
9ccee9cd02 doc: Document WITH_EXTERNAL_LIBMULTIPROCESS build option better (Ryan Ofsky)
Pull request description:
This includes a cmake documentation change suggested https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2039716492 and another change to mention the option in markdown documentation
ACKs for top commit:
hebasto:
ACK 9ccee9cd02, changes look good.
TheCharlatan:
ACK 9ccee9cd02
Tree-SHA512: c9103b001b970ac57afedc6dc384091f5661975d569573e93003cbd7df1891c54cefb06d7296eac5b9a5c57251803dcab2bd3b26c9d81aa476c62f211dcb3d6e
27f11217ca ci: drop -priority-level from bench in win cross CI (fanquake)
Pull request description:
So there's at least one CI sanity checking all benchmarks.
Related to #32277.
ACKs for top commit:
l0rinc:
utACK 27f11217ca
hebasto:
ACK 27f11217ca.
mabu44:
utACK 27f11217ca
Tree-SHA512: 4853584bf9db418f6e31aa0f558d08bc45479d672b193e1d25a25907f82fb225bc4388321f8f23286cd9fd9168c7546c713829607eb0cf5e3c62b98e88f8e68b
