Benchmark 32-byte hashes and inputs consisting of a 32-byte hash plus a 32-bit index with SipHash-2-4 and SipHash-1-3-UJ.
The UJ benchmark zero-extends the index into one 64-bit normal block.
Keep all four measurements together after shared correctness coverage and before changing `CCoinsMap`'s hasher.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Add SipHash-1-3-UJ outputs to the shared vectors for sequences of 8- and 32-byte blocks.
Check generic writes and applicable fixed-width `Hash` overloads against those outputs, and fuzz their equivalence including mixed normal/jumbo encodings.
The outputs were generated by an independent implementation that Claude Opus 4.8 produced using only the `SipHasher13UJ` class comment as its prompt.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Add const `SipHasher13UJ::Hash` overloads for one `uint256` jumbo block, optionally followed by one 64-bit normal block.
Reuse the generic hasher's initialized state and define these short paths in the header because the new hasher benefits from inlining.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Add `SipHasher13UJ`, a block-oriented implementation accepting arbitrarily mixed 64-bit normal blocks and 256-bit jumbo blocks.
For hash-table use, cryptographic hash outputs must make up all but a small bounded number of retained jumbo blocks.
The generic interface serves as the executable specification for the fixed-width overloads added next.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Co-authored-by: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
Move the SipHash round, compression, and finalization operations shared by `CSipHasher` and `PresaltedSipHasher` into inline `SipHashState` methods.
This centralizes state mutation, preserves the existing byte-path code generation, and keeps the security-sensitive follow-up focused on its changed block compression, round counts, and finalizer.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Lock SipHash-2-4 behavior into shared vectors before refactoring its round and finalization code.
Store inputs as ordered hex byte blocks so `CSipHasher` and the independent Python implementation hash the same byte sequence, with applicable `PresaltedSipHasher` overloads checked against the same vectors.
Add the 64 official SipHash-2-4 vectors alongside block-partition and empty-block cases for the generic path.
Move randomized generic/fixed comparisons to the integer fuzzer.
SipHash-1-3-UJ coverage can add expected outputs for compatible 8- and 32-byte block sequences.
The Python test reads a build-tree copy so functional-test staging behaves consistently when files are symlinked or copied.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
6ee05c4b18 test: wallet: BnB incomplete result on attempt-limit success (Bruno Garcia)
Pull request description:
BnB can return a valid selection before exhausting the search tree, then hit `TOTAL_TRIES` while continuing to look for a better one. Add a unit test for that path using a known exhaustion fixture plus an exact-match coin, and assert the result is marked incomplete via `GetAlgoCompleted() == false`.
It kills the following mutant:
```diff
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index 8d69957c30..e4d07415f3 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -212,7 +212,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
if (curr_try >= TOTAL_TRIES) {
// Solution is not guaranteed to be optimal if `curr_try` hit TOTAL_TRIES
- result.SetAlgoCompleted(false);
+ result.SetAlgoCompleted(true);
break;
}
```
ACKs for top commit:
yashbhutwala:
tACK 6ee05c4b18
achow101:
ACK 6ee05c4b18
murchandamus:
ACK 6ee05c4b18
Tree-SHA512: 32d0cc6da7586abdd195b87459da0a6ebbdcf8115bef41f5e625864bfa9ce8231a58f8d4b61708279ebeaef372e4f3b5fb18eab28a01dad53f043ba083330b3d
BnB can return a valid selection before exhausting
the search tree, then hit TOTAL_TRIES while continuing
to look for a better one. Add a unit test for that path
using a known exhaustion fixture plus an exact-match
coin, and assert the result is marked incomplete via
GetAlgoCompleted() == false.
Co-authored-by: Murch <murch@murch.one>
3e8e21b2ef txgraph: avoid moving primitive members (Lőrinc)
d9f94aa882 rpc: avoid moving RPC enum types (Lőrinc)
b67baed4e7 coins: avoid moving `COutPoint` values (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/34320#discussion_r2751764873.
**Problem:** A few code paths use rvalue references or `std::move()` for types where moving provides no benefit.
`EmplaceCoinInternalDANGER` took `COutPoint&&`, forcing callers to pass trivially copyable outpoints as rvalues even though the cache stores its own key.
Some call sites also use `std::move()` on enum and primitive values, where it only adds noise.
> [!NOTE]
> `CheckTriviallyCopyableMove` remains `false` since `std::move()` on trivially copyable types can still be useful as intent documentation, for example to signal that a value should not be reused after a call.
**Fix:** Take trivially copyable arguments by const reference where the callee only needs to store its own copy, and pass existing values directly at the call sites.
Also remove `std::move()` from enum and primitive assignments where it has no semantic effect.
ACKs for top commit:
maflcko:
review ACK 3e8e21b2ef 🖇
hodlinator:
re-ACK 3e8e21b2ef
andrewtoth:
ACK 3e8e21b2ef
hebasto:
ACK 3e8e21b2ef, I have reviewed the code and it looks OK.
Tree-SHA512: cbe55b13290ae261bba359dc6e5a3bbdfb7ae9d31bdf8e0da2eef65a0df776e4081ceeac3c82731a631bda656a86b3789651fa1f4d87875cee1dc96351bdfd7c
Serialization parameters should be embedded into the object being
serialized rather than passed as a separate argument. This works here
because only serialization is performed and no new object needs to be
constructed.
dab7f2c984 test: cover -externalip/onlynet interaction in functional test (will)
657a5aa3f3 test: cover -externalip bypassing -onlynet (will)
8c87e32bd3 net: let -externalip bypass -onlynet (will)
f4af02e827 net: add an add_even_if_unreachable argument to AddLocal (will)
Pull request description:
`-onlynet` is documented to restrict automatic outbound connections, but it also currently prevents `-externalip` addresses from being advertised when their network is not in the `-onlynet` set. This happens because `AddLocal()` rejects addresses outside `g_reachable_nets`, regardless of whether the address was explicitly configured by the user.
Previous attempts to fix this (#24835 and #25690) removed the `g_reachable_nets` check from `AddLocal()`.
This PR instead adds an explicit `add_even_if_unreachable` argument to `AddLocal()`. The argument defaults to `false`, and is set to `true` only when adding addresses from `-externalip`.
As a result, explicitly configured `-externalip` addresses can still be advertised even when their network is excluded by `-onlynet`, while discovered, mapped, bound, Tor control, and I2P SAM addresses continue to use the existing reachable-network filter.
This keeps the fix scoped to `-externalip` and addresses the concern raised in #25690:
> I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network
The branch adds unit coverage for `AddLocal()` and functional coverage in `p2p_addr_selfannouncement.py` for `-onlynet=ipv4 -externalip=<onion>`.
Fixes: #25336Fixes: #25669
ACKs for top commit:
achow101:
ACK dab7f2c984
mzumsande:
re-ACK dab7f2c984
w0xlt:
ACK dab7f2c984
Tree-SHA512: a4ac9334b85da8b6902d3850e21d3a1c9d7dce70bcb79182448c8d5684e24462cd6e440385af7aa4420d9582e4dff9dc9e827ca7a6da0363fff2d3c531784d9b
4c9de7d5b3 external_signer: validate fingerprint from enumerate response (Kyle 🐆)
Pull request description:
`enumeratesigners` takes the `fingerprint` field from the external signer's `enumerate` output and stores it without checking it. That value is later handed back to the signer command as `--fingerprint <value>` (e.g. in `displayaddress`), so a malformed value propagates unchecked.
A master key fingerprint is 4 bytes, i.e. 8 hex characters. This adds a check that the reported fingerprint is exactly 8 hex characters and throws a clear error otherwise. A functional test covers empty, wrong-length, and non-hex inputs.
ACKs for top commit:
Sjors:
utACK 4c9de7d5b3
achow101:
ACK 4c9de7d5b3
sedited:
ACK 4c9de7d5b3
Tree-SHA512: 7c3303b24e234e13a4c20c0b93552145b9ccffc29d1bae42ce8a2faf548377f051e52f8ffb3924679065b27d15fc7bf3859e5ae32a2bb185738cc29bc0ade486
72db4accbf coins: drop stale cursor null checks (Lőrinc)
3d2f2d8de0 coins: pass UTXO stats view by reference (Lőrinc)
35aedb2823 coins: drop cursor from base view (Lőrinc)
c6fbe2f66c coins: pass DB view to cursor users (Lőrinc)
Pull request description:
**Problem:** `CCoinsView::Cursor()` makes cursor iteration look like a generic coins view operation, but cursor iteration is only supported by the DB-backed coins view.
The cache override only threw, and the `coins_view` fuzz target only asserted that deterministic unsupported throw path.
**Fix:** Make cursor iteration a `CCoinsViewDB` operation.
Cursor users now take the DB-backed view directly, `CCoinsView` no longer exposes `Cursor()`, and the fuzz target keeps DB-backed cursor coverage while dropping the unsupported cache throw probe.
The UTXO stats path is also tightened to pass the non-null DB view by reference, and stale null handling for DB cursors is removed.
This was extracted from review discussion in https://github.com/bitcoin/bitcoin/pull/35295#discussion_r3420576781 and extended based on https://github.com/bitcoin/bitcoin/pull/35562#issuecomment-4746585893.
ACKs for top commit:
achow101:
ACK 72db4accbf
sedited:
Re-ACK 72db4accbf
w0xlt:
ACK 72db4accbf
andrewtoth:
ACK 72db4accbf
Tree-SHA512: 12a81330a6ec1b91a7e4393f3761ea9ed4702ecb24312f1defa5a9a079a396ce921fc52f74fe296e5ac7ab20d5b5a8a84e858c96847f333c58b7fa9de9e8143e
3ae3a94f2b wallet: avoid call bumpfeediscount with negative values (Pol Espinasa)
Pull request description:
in https://github.com/bitcoin/bitcoin/issues/34232 dergoegge reported an assertion fail in `SetBumpFeeDiscount`.
**Context**
The bump-fee discount in the savings that we can have when multiple UTXOs that we selected for our transaction share a common unconfirmed ancestor transaction.
We know we can have some savings because we first calculate the `summed_bump_fees` and then compare to the `combined_bump_fee`.
For context:
- `bump_fees`: Extra fees that the new transaction must pay to contribute to get his ancestor confirmed.
- `summed_bump_fees`: The sum of each input ancestor `bump_fee`. If we have two inputs with unconfirmed ancestors A and B and both have a `bump_fee = 100` then `summed_bump_fees = 200`.
- `combined_bump_fee`: Is the total `bump_fees` summed of all inputs, but taking into account shared ancestors. If A and B share the transaction ancestor then `combined_bump_fee = 100` not `200` as the transaction must be bumped only once.
If `summed_bumpfees` > `combined_bump_fee` we are overestimating the `bump_fee` as we are counting multiple times the same ancestors so we can discount it using `SetBumpFeeDiscount(summed_bump_fees - combined_bump_fees)`.
**Problem**
To calculate `summed_bump_fees` and `combined_bump_fee` we use two different fresh MiniMiner snapshots of the mempool. Because they are called in different moments the two snapshots of the mempool might be different. An artificial feerate decrease of an ancestor using `prioritizesettransaction` can make `combined_bump_fee > summed_bump_fees` creating a negative discount. This cause calling `SetBumpFeeDiscount` with a negative vaule triggering an assertion `discount >= 0`.
**Fix**
This PR fixes it by ensuring not only that a discount exist but also that is greater the 0.
**Test**
It is hard to manually trigger this race condition. dergoegge coded a patch and test to trigger it that can be used to test the fix.
5320e2fd21
ACKs for top commit:
achow101:
ACK 3ae3a94f2b
pablomartin4btc:
ACK 3ae3a94f2b
Tree-SHA512: e76693eb66c4883ed3ef5edf1baa972657b0d532487803706312d486b4d4a4997f5dcbc64781a1a21b7b5628b06fcc97a6633382509061734d0f5615e851bef1
Remove the `Mutex` from the `coins_view` and `coinscache_sim`
pool startup helpers. Fuzz targets are entered sequentially within a
process and parallel fuzzing uses separate processes/forks, which each
have their own copy of the global thread pool. Therefore, a mutex to
prevent two in-process callers from racing to start the pool isn't needed.
fab8eeed82 fuzz: clang-format LIMITED_WHILE (MarcoFalke)
fa0d777ce2 fuzz: Clang-format LIMITED_WHILE like while (MarcoFalke)
fa1a9bde5a fuzz: Remove unused workaround after fix in libmultiprocess byte-span serializer (MarcoFalke)
fa55385ab3 fuzz: Use LIMITED_WHILE over for-loop with consumed size integral (MarcoFalke)
6d5f753921 Squashed 'src/ipc/libmultiprocess/' changes from 28e056576a..e8de5c7b68 (MarcoFalke)
Pull request description:
Includes several changes, to first update the subtree. Then, modify the fuzz test to address review comments:
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3506566815
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3523175145
ACKs for top commit:
ryanofsky:
Code review ACK fab8eeed82. Just fuzz test clang-format cleanups added since last review, which seem nice
Tree-SHA512: 0836628f8ee54adf02571025456211a74f63d05058b72280b10111ecfbb93d30945f4a48f30cc790774de4e2b489907313e86b0fa6729f3480c64850c94848b4
6667dc4eec kernel: expose scriptSig for btck_TransactionInput (Peter Zafonte)
e6de3a2d3c kernel: expose witness stack for btck_TransactionInput (Peter Zafonte)
Pull request description:
Silent payments scanning needs the public key from every input. For SegWit inputs it is in the witness stack. For P2PKH inputs it is in scriptSig. Without these new functions, callers must deserialize the raw transaction themselves to reach that data, which is difficult and error-prone.
Introduces a `btck_WitnessStack` type and adds the following functions:
**Witness stack:**
`btck_transaction_input_get_witness_stack`: returns a non-owning `const btck_WitnessStack* `view
`btck_witness_stack_count_items `: item count
`btck_witness_stack_get_item_at `: single item by index via btck_WriteBytes
`btck_witness_stack_copy` / `btck_witness_stack_destroy`: lifecycle for owned copies
**scriptSig:**
`btck_transaction_input_get_script_sig`: full scriptSig via btck_WriteBytes
All functions are exposed in the C++ wrapper via `WitnessStackView`, `WitnessStack`, and `WitnessStackApi` , and `GetScriptSig()`.
ACKs for top commit:
sedited:
ACK 6667dc4eec
musaHaruna:
ACK [6667dc](6667dc4eec)
stickies-v:
ACK 6667dc4eec
Tree-SHA512: b5e9d32ec87a5f5a9fea5652ed69737eae1d1f9cfb777544b96d703bbd057c114e0b7afae4e9e9d09b8b546694b14fb923b747c04a8f0211ab6caa015d06967d
This is a whitespace-only clang-format change.
To verify it, one can run:
```sh
(git show | git apply --reverse ) && ( git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v ) && git diff HEAD
```
A few minor, non-macro formatting adjustments were made in touched files:
* `src/wallet/test/fuzz/crypter.cpp`: Removed a redundant double semicolon
* `src/test/fuzz/txorphan.cpp`: Corrected indentation on an `else if` block.
* `src/test/fuzz/mini_miner.cpp`: Removed an unnecessary empty line.
This is a style cleanup. The general pattern to use `LIMITED_WHILE`,
which all other fuzz tests use, has some benefits:
* When no data is available, a simple and single (let's say) 64 value in
the fuzz input will not result in 64 loops over the same body with the
same default/fallback values.
* When no data is available, `ConsumeBool` falls back to `false` and
breaks the loop early.
* When further data is available, the overhead is just a single byte,
making it also possibly easier for the fuzz engine to mutate the data,
as a single int that influences the whole remainder of the fuzz input
can lead to the 'havoc' effect.
* When a crash is reduced, deleting bytes will directly influence the
execution length, so byte-length of the fuzz input roughly corresponds
to run-time length.
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
463d073cb8 test: rename vBool to vector_bool
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization
git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
d164a04342 node: smooth oversized `-dbcache` warnings (Lőrinc)
Pull request description:
**Problem:** The oversized `-dbcache` warning threshold has a sharp formula cliff when detected RAM crosses the cutoff used by the warning logic.
This was reported during review of [#34641](https://github.com/bitcoin/bitcoin/pull/34641#discussion_r2900769756), where an earlier version could jump from the auto default at `4095 MiB` RAM to `75%` of RAM at `4096 MiB`.
That made `1 MiB` of extra detected RAM raise the warning threshold from about `511 MiB` to `3072 MiB`.
The surviving warning-only code has the same shape at a different boundary.
Below `2 GiB` RAM the cap is `DEFAULT_DB_CACHE` (`450 MiB`), but at `2 GiB` it switches to `75%` of total RAM, so a tiny increase in detected RAM can suddenly raise the warning threshold from `450 MiB` to about `1536 MiB`.
<img width="1484" height="881" alt="Image" src="https://github.com/user-attachments/assets/b51d5d24-31b8-4a2a-8f70-e7536481f855" />
**Fix:** Base the warning on a reserved non-dbcache memory budget instead:
```math
\text{warn if } \mathit{dbcache} > \max\left(\mathit{DEFAULT\_DB\_CACHE}, 0.75 \cdot \max(\text{total RAM} - \mathit{DBCACHE\_WARNING\_RESERVED\_RAM}, 0)\right)
```
`DBCACHE_WARNING_RESERVED_RAM` is `2 GiB`, so the fixed `DEFAULT_DB_CACHE` cap remains the floor below that reserve and the warning threshold grows monotonically above it.
This keeps the warning conservative around low-memory boundaries and avoids treating a boundary-crossing `1 MiB` RAM difference as a reason to allow a much larger explicit `-dbcache`.
**Quick reference:**
| System RAM | Previous warning cap | New warning cap |
| ---------- | -------------------- | --------------- |
| 1 GiB | 450 MiB | 450 MiB |
| 2 GiB | 1536 MiB | 450 MiB |
| 3 GiB | 2304 MiB | 768 MiB |
| 4 GiB | 3072 MiB | 1536 MiB |
| 8 GiB | 6144 MiB | 4608 MiB |
| 16 GiB | 12288 MiB | 10752 MiB |
| 32 GiB | 24576 MiB | 23040 MiB |
On 32-bit builds, effective `-dbcache` values are still capped to `1024 MiB` before the warning check, so thresholds above that cap are not reachable there.
ACKs for top commit:
optout21:
reACK d164a04342
sedited:
Re-ACK d164a04342
w0xlt:
reACK d164a04342
Tree-SHA512: deb81f0e192261f01dda6f1575a7b2e147f1e07e813d0833c7aeaf6a3fbd7594c0145ec1c98ded74efee9005108d229942949279c9458cbec4046913387db845
9e62e4b1f3 test: slow down rescaning process (Pol Espinasa)
336f5a738b wallet: reserve walletrescan before checking wallet is at the tip (Pol Espinasa)
Pull request description:
`ImportDescriptors` rpc has a race condition where two imports running in parallel can both succeed or fail one of them.
The race happens when there are two threads A and B trying to importdescriptors at the same time.
1. Thread A calls `BlockUntilSyncedToCurrentChain()` (holding `cs_wallet` fast, no contention) and then `reserve()`, acquiring the `WalletRescanReserver`. It proceeds to `ProcessDescriptorImport()`, which holds `cs_wallet` for an extended time (specially on slow machines) while importing descriptors.
2. B reaches `BlockUntilSyncedToCurrentChain()`, which internally does `WITH_LOCK(cs_wallet, ...)`. Since A holds `cs_wallet`, B blocks here for the entire duration of Thread A's descriptors import.
3. Then A finishes importing, releases `cs_wallet`, rescans (fast in regtest), and sets `fScanningWallet = false`.
4. B can now continue in `BlockUntilSyncedToCurrentChain()` acquiring `cs_wallet`, and then calls `reserve()` which succeeds because `fScanningWallet` is already `false`. Both imports succeed.
I don't think the behavior is problematic at all from a usability PoV, but it can be a bad UX if some imports fails and other's no. It also makes testing difficult as race conditions are not easy to test.
This PR fixes it by calling `reserver.reserve()` before `cs_wallet` is locked, so multiple threads will be aware of currently imports before being stuck at any point. So only one `importdescriptor` call can be done at the same time.
I think this should fix https://github.com/bitcoin/bitcoin/issues/35544#issuecomment-4763488259
ACKs for top commit:
achow101:
ACK 9e62e4b1f3
nebula-21:
ACK 9e62e4b1f3
w0xlt:
lgtm reACK 9e62e4b1f3
Tree-SHA512: be0027e1a7b77252ed9fb514c3b3311d6905903d4b0bfc1021a1e1c2bb06872ef599647ff8a4536929240717ae9db62fb75374f629cd3046c7192e2b8b4d7344
db35b9238f ipc # build: Fix fuzz target CMakeLists.txt for external libmultiprocess (Ryan Ofsky)
Pull request description:
CMake `WITH_EXTERNAL_LIBMULTIPROCESS` and `BUILD_FUZZ_BINARY` options stopped working together recently due to #35118 commit 037ad77071 because an non-namespaced `Libmultiprocess::multiprocess` target name was referenced.
Fix by specifying the full target name which is better for readability anyway.
ACKs for top commit:
sedited:
ACK db35b9238f
hebasto:
ACK db35b9238f, I have reviewed the code and it looks OK.
Tree-SHA512: 7cfb19a66dd4ccdcd6dfdedc3000fd20f488851f3235852717fd31698a7e8efb5c4e14705b0dc0c1ea506b38268f1ab030bca7865ddfdd85144598f435581247
fed3cf6f0e wallet: Replace CWalletTx's vOrderForm with specific fields (Ava Chow)
4f8823e8e1 wallet: Drop vOrderForm from CommitTransaction (Ava Chow)
a2b0bfcd85 wallet: Drop mapValue from CWalletTx (Ava Chow)
cb99864c91 wallet: Throw if unknown entry is found in mapValue (Ava Chow)
98d5cdae66 wallet: Make CWalletTx "replaces_txid" and "replaced_by_txid" member variables (Ava Chow)
7ef8a6efc2 wallet: Make CWalletTx "comment" and "to" member variables (Ava Chow)
2155e913d3 wallet: Make CWalletTx "from" and "message" member variables (Ava Chow)
c6ba98dcc8 wallet: Drop mapValue from CommitTransaction (Ava Chow)
00abb174a8 wallet: Pass comment and comment_to to CommitTransaction (Ava Chow)
1a219a37a2 wallet: Pass replaces_txid to CommitTransaction outside of mapValue (Ava Chow)
Pull request description:
`mapValue` and `vOrderForm` are opaque data structures that contain transaction metadata. It is hard to determine what actual data each field contains, and they can ostensibly be misused where metadata is added in the future without developers realizing that such metadata exists.
It's much clearer to have all of that metadata live in their own explicit member variables within `CWalletTx`. This PR implements that change.
Since the serialization format of `CWalletTx` depends on `mapValue` and `vOrderForm`, the serialization remains unchanged, so when serializing these new members, they need to be shoved/extracted from a temporary `mapValue` or `vOrderForm`.
This does end up breaking forwards compatibility as unknown fields in `mapValue` and `vOrderForm` are stripped out if the record is rewritten. However, I don't expect that we would continue to use these fields for future metadata, so I think that risk is low.
ACKs for top commit:
ajtowns:
reACK fed3cf6f0e
w0xlt:
ACK fed3cf6f0e with above nits/caveats.
Eunovo:
ACK fed3cf6f0e
Tree-SHA512: c7deab5aaeac13656012f8b13c0161fd420d2a5348eebd7649310e78ccb1216995aa6a7cbd506ac8d11d7b46b0856d6e6a897bc39965b51cfcf2268356ace261
a8223bb4e6 wallet: Introduce WalletError with machine-readable error code (pseudoramdom)
Pull request description:
Per discussion in https://github.com/bitcoin/bitcoin/pull/35436#issuecomment-4923786884, `WalletError` is split out so that it can be reused by multiple wallet interface changes (#34861 in particular)
----
Introduce a `wallet::WalletError`, a generic wallet-layer error type that contains
- a machine-readable `WalletErrorCode` for programmatic handling
- a translated user-facing `bilingual_str` message
The initial enum is intentionally small. `WALLET_ERROR` is used for generic failures that callers should display to the user. The intention is to have more specific codes only when the callers can handle the condition differently.
ACKs for top commit:
achow101:
ACK a8223bb4e6
davidgumberg:
ACK a8223bb4e6
polespinasa:
ACK a8223bb4e6
Tree-SHA512: 15fedf96cb5c3e8167a236bfa1a4d01d94f258a22943e53330ab432ad04a31f81f2eac8749f38390b2d34156d283d3bf76f293c640724bd0b090dc494123b230
2bab6bc73f refactor: Drop support for FreeBSD < 14 (Hennadii Stepanov)
91b5c8a07c refactor: Remove FreeBSD-specific workaround (Hennadii Stepanov)
56701ff6d5 doc: Clarify supported *BSD releases (Hennadii Stepanov)
Pull request description:
This PR establishes a baseline for the oldest *BSD releases supported by Bitcoin Core. Clarifying these minimum requirements paves the way for dropping compatibility code and workarounds for unsupported versions.
The obsolete FreeBSD-specific workaround and version check have been dropped.
ACKs for top commit:
maflcko:
lgtm ACK 2bab6bc73f
willcl-ark:
ACK 2bab6bc73f
theStack:
lgtm ACK 2bab6bc73f
sedited:
ACK 2bab6bc73f
Tree-SHA512: 6d9ca0ff881a60c33fe3aa18a03726426f07f2896b2f56b12804865acfa910aca7efdc1312eb4055e35aab8423d0c2326b89c1da448e01b4fa213f73dfd2b118
a2e4cd7ad2 depends: capnp 1.5.0 (fanquake)
Pull request description:
Update capnp in depends to [`1.5.0`](https://github.com/capnproto/capnproto/releases/tag/v1.5.0), which contains numerous security and bugfixes. More details here: https://github.com/capnproto/capnproto/blob/v2/security-advisories/2026-07-09-capnproto-v1.5-rollup.md:
> Like many projects, in recent months, Cap'n Proto has experienced an uptick in security reports, almost certainly driven by the use of AI to find vulnerabilities.
> In the past, I have always issued a separate security advisory with a CVE for each and every bug. However, this is time-consuming, and I just don't have the bandwidth to keep up. Therefore, given the quantity of bugs reported, I have opted to issue a single combined security advisory for this release.
>According to CVE rules, each distinct bug must have a separate CVE. Roll-ups are not allowed. Unfortunately, this means I cannot request a CVE for this advisory.
See https://github.com/capnproto/capnproto/compare/release-1.4.0...release-1.5.0 for all changes since 1.4.0.
ACKs for top commit:
hebasto:
ACK a2e4cd7ad2.
janb84:
ACK a2e4cd7ad2
Tree-SHA512: e616ab88a7692f17611865fbb36fc0c598ff6d8bcf18035f22d84681bbda8b6f5e7b8d7270baeeaf0d18e035de474003345f3bd75f020e615388ae04b3dd22f4
7508ac319d bench: replace CreateMockableWalletDatabase with MakeInMemoryWalletDatabase (Pablo Martin)
Pull request description:
Benchmarks don't need mock-specific behaviour (overridden `Filename()`, `Format()`, or the exposed batch-level `WriteKey()`). Replace `CreateMockableWalletDatabase()` with `MakeInMemoryWalletDatabase()` across all 6 call sites in `src/bench/` (5 files - 4 wallet bench files + 1 in coin_selection), using the same in-memory SQLite path that production code uses.
`wallet_migration.cpp` is excluded: it calls `GetOrCreateLegacyDataSPKM()` which asserts `Format() == "sqlite-mock"`, a deliberate signal that allows legacy SPKM setup in `test/bench` contexts. `MockableSQLiteDatabase` is still correct there.
For `coin_selection.cpp`, which had no other dependencies on `wallet/test/util.h`, the include is switched to `<wallet/sqlite.h>`. The remaining 4 files retain `wallet/test/util.h` for other utilities but also add an explicit `<wallet/sqlite.h>` include as required by IWYU.
Follow-up suggested in #35655.
ACKs for top commit:
sedited:
ACK 7508ac319d
janb84:
ACK 7508ac319d
Tree-SHA512: 6f4086eb5700ba3da882378dcee76b3c075670cdea5d71b45c9b369e48c72b8f591e0c5a6bd4504719dd512b3b7d2b23230a8b76e471c6c0571c13325fa3cba0
**What failed:** Build error: 'mp/proxy-io.h' file not found when
building the ipc fuzz target.
**Root cause:** Upstream commit 037ad77071 (fuzz: add IPC round-trip
target) added target_link_libraries(fuzz bitcoin_ipc_fuzz multiprocess)
using the bare 'multiprocess' target, which only exists in subtree
builds. With WITH_EXTERNAL_LIBMULTIPROCESS=ON, no 'multiprocess' cmake
target is defined, so the INTERFACE_INCLUDE_DIRECTORIES from the
installed libmultiprocess headers were never propagated to the fuzz
target, causing the missing-header error.
**Fix:** Changed to 'Libmultiprocess::multiprocess', which is the
canonical namespaced target available in both subtree and external builds.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fad5809cb9 doc: Update enum class constant naming style guide (MarcoFalke)
Pull request description:
Lately, it seems there are frequent scripted-diffs and refactors to rename ALL_CAPS enum class constant names to something else, due to third-party macro clashes. E.g.:
* https://github.com/bitcoin/bitcoin/pull/35588
* https://github.com/bitcoin/bitcoin/pull/35487
* https://github.com/bitcoin/bitcoin/pull/34454
* etc... (not listing the intermittent pull request force pushes that lead to early CI failures due to macro clashes)
Try to steer away from ALL_CAPS here by discouraging it in new code.
ACKs for top commit:
kevkevinpal:
ACK [fad5809](fad5809cb9)
hebasto:
ACK fad5809cb9.
pablomartin4btc:
ACK fad5809cb9
stickies-v:
ACK fad5809cb9
yuvicc:
ACK fad5809cb9
musaHaruna:
ACK [fad5809](fad5809cb9)
janb84:
ACK fad5809cb9
Tree-SHA512: f652c0127022a5ea131e956aef0a2d8c98f4c4317519475a38e5f527a5f672b93a8e51743c1a588f38ddc795531168b68ab68f86471b61f25229761eff0f3879
Benchmarks don't need mock-specific behaviour (overridden Filename(),
Format(), or the exposed batch-level WriteKey()). Replace
CreateMockableWalletDatabase() with MakeInMemoryWalletDatabase() across
6 call sites in src/bench/ (5 files), using the same in-memory SQLite
path that production code uses.
wallet_migration.cpp is excluded: it calls GetOrCreateLegacyDataSPKM()
which asserts Format() == "sqlite-mock", a signal used to allow legacy
SPKM setup in test/bench contexts. MockableSQLiteDatabase is still
correct there.
For coin_selection.cpp, which had no other dependencies on
wallet/test/util.h, also switch the include to <wallet/sqlite.h>.
Follow-up suggested in #35655.
63c5f9d22c test: Remove `mock_process.cpp` (rustaceanrob)
Pull request description:
Picked from #35587, but I think has motivation to go in on its own.
The previous binary used a number of `Boost.Test` features:
- `boost::unit_test::disable`
- `BOOST_FAIL`
- `boost::exit_test_failure`
This patch duplicates the previous mock process behavior with no boost features.
With the patch we can:
- simplify the test config
- remove a linted boost include
- remove a file that was not actually a test
ACKs for top commit:
kevkevinpal:
ACK [63c5f9d](63c5f9d22c)
maflcko:
review ACK 63c5f9d22c🧀
hebasto:
ACK 63c5f9d22c.
Tree-SHA512: 15ff5ad49256149bb419beb72f002dd55fc62139e61fd80d2cecceff2699f291a5c14b39b027c412352aa965ec320bac320be32170e6d9d944b43e14f4864c35
6d0ea4cf5b doc: add release notes (Andrew Toth)
a2b1c86903 txospenderindex: disable bloom filters to optimize disk usage (Andrew Toth)
Pull request description:
LevelDB bloom filters are only consulted on `Get` point reads. This can be verified in https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/table/table.cc#L224-L228. `InternalGet` is the only place that consults the filter, and it is only reached via a `Get` or `Exists` point read. The filters are never consulted for iterator seeks with an iterator created via `NewIterator`.
txospenderindex only reads via iterator seeks, so building them is wasted effort and space.
For a db as large as txospenderindex, this results in measurable performance and disk usage.
On master, a full sync took 4h37m, and the resulting db was 85.0 GiB.
On this branch, a full sync took 3h57m, and the resulting db was 80.9 GiB.
So this is a sync speedup of 39 minutes (1.17x), and a disk space reduction of 4.2 GiB.
ACKs for top commit:
l0rinc:
ACK 6d0ea4cf5b
sedited:
Re-ACK 6d0ea4cf5b
fjahr:
Code review ACK 6d0ea4cf5b
Tree-SHA512: fb88b9f9a16ff31562d388e3fd9fd9590c7864dbe6093cd9430ecbce9cdc3f2a8d3fc612aade743d26ad4c6eca1e5dc9b3f1ca28d75caea1209e5c784895405d
The previous binary used a number of `Boost.Test` features:
- `boost::unit_test::disable`
- `BOOST_FAIL`
- `boost::exit_test_failure`
This patch duplicates the previous mock process behavior with no boost features.
With the patch we can:
- simplify the test config
- remove a linted boost include
- remove a file that was not actually a test
777d23f25c test: add regression test for in-memory SQLiteDatabase reopen (Pablo Martin)
d1e7f8c986 wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet (Pablo Martin)
ee43743f12 wallet: store m_additional_flags in SQLiteDatabase to fix reopen path (Pablo Martin)
Pull request description:
Since #33032 landed (in-memory `SQLiteDatabase` via `SQLITE_OPEN_MEMORY`), the intermediate wallet built during `exportwatchonlywallet` can live entirely in memory instead of being written to the wallets directory as a temporary file.
The temp wallet is a pure build artifact: it is populated with descriptors, transactions, and address book data, then immediately discarded once `BackupWallet()` copies its contents to the destination file. Making it in-memory removes all on-disk footprint and eliminates the `cleanup_watchonly_wallet` RAII handler — along with the `wallet_path` and `cleanup_files` variables it needed — which previously ensured the temp files were deleted on both success and failure paths.
This PR introduces `InMemoryWalletDatabase` (a minimal `SQLiteDatabase` subclass) and `MakeInMemoryWalletDatabase()` factory in `sqlite.h/cpp`, following the same pattern as `MockableSQLiteDatabase` / `CreateMockableWalletDatabase()`. `MockableSQLiteDatabase` now derives from `InMemoryWalletDatabase`, removing its redundant `Files()` override.
Suggested by Sjors in #32489 ([comment](https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-4874894955)).
---
Also fixes a related issue found (by Sjors) during review:
- `SQLiteDatabase::Open()` (the no-arg public override) hardcoded 0 as `additional_flags` when reopening after a failed `TxnAbort()`, which would reopen an in-memory database as on-disk. Fixed by storing `m_additional_flags` in the constructor and using it in the reopen path. For in-memory databases, both the `force_conn_refresh` path and the public `Open()` now throw instead of silently creating a fresh empty connection. A regression test for the `Open()` throw is included in a separate commit.
---
As a follow-up, `InMemoryWalletDatabase` could replace `MockableSQLiteDatabase` in `src/bench/` (5 files, 6 call sites), since benchmarks don't need mock-specific behaviour and benefit from using the same in-memory path as production code.
ACKs for top commit:
Sjors:
re-utACK 777d23f25c
achow101:
ACK 777d23f25c
janb84:
ACK 777d23f25c
Tree-SHA512: 71178ce99c7ebc0fc5ba17956c27d37f90e3d36cefbdc0d15d1424b7a70bf15f05a13cb03c268d885678aba1fd3c90567d8389d3680650c8ae46f4cb4b10b28a
