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
c43b7a1115 ci: add netBSD cross CI job (fanquake)
699c21aea4 depends: add netbsd_LDFLAGS (fanquake)
Pull request description:
This adds a Linux cross job for NetBSD; similar to https://github.com/bitcoin/bitcoin/pull/34491 (FreeBSD).
This uses the upcoming NetBSD 11.0 release (rc5).
ACKs for top commit:
willcl-ark:
ACK c43b7a1115
Tree-SHA512: 717b5c68e9cf4dd2b6f9101320ebe94df5047745445c2698005d0dfa034776c2fa836b599155c7cb7e93024b15ba7b91cef07275185cd1c07f58fea678e2354d
629df81e4c Remove myself as security contact (Pieter Wuille)
Pull request description:
Making the same change here as in https://github.com/bitcoin-core/bitcoincore.org/pull/1264. I remain involved in security discussions, but don't feel like functioning as a first-line contact anymore.
ACKs for top commit:
achow101:
ACK 629df81e4c
fanquake:
ACK 629df81e4c
Tree-SHA512: b76813b7e9679cee60a9dd1e8d8de46cd8e329314c677fd4834bde415f8b1f351ebee4eb4c885e889b8a111e8d46d2d20af6d5a4ff108c64df29874511318c2f
d908063980 chainparams: delete my DNS seed (Pieter Wuille)
Pull request description:
I plan to shut down my DNS seed server in the future (probably after the release of 32.0), so I request it be removed from the master branch already. It has existed for almost 15 years at this point, and isn't getting much maintenance attention from me anymore.
ACKs for top commit:
l0rinc:
ACK d908063980
Sjors:
ACK d908063980
pinheadmz:
ACK d908063980
janb84:
ACK d908063980
Tree-SHA512: b3f5f0361d00557b94987a36a8557cf5c6f0ad1415d5ae6b70d64bce55f45f0a3d4c137b9509270128b1ef97b5ef0a3c67e4bbe6c669d5e4a79bd787cc7801c7
Introduce WalletError as a generic wallet-layer error type that can carry a machine-readable WalletErrorCode and a translated user-facing message.
The WalletErrorCode::GenericError code is intended for failures that callers should only display to the user. More specific codes should only be added when callers can handle the condition differently.
e9ed898a0d validation: Don't use m_chain.Tip() in FlushStateToDisk (Martin Zumsande)
3679f1ecf5 index: Don't commit ahead of the flushed chainstate (Martin Zumsande)
65735728a5 index: Remove return value from Commit() (Martin Zumsande)
09c06960c6 validation: track last flushed block (Martin Zumsande)
13c02b5466 test: add test for index commits ahead of the last flushed block (Martin Zumsande)
Pull request description:
If indexes commit their data ahead of the flushed chainstate, and there is an unclean shutdown, the index will be corrupted. This is especially the case for the coinstatsindex, which has state (the muhash) which can't easily be rolled back without access to the blocks. This was only partly fixed in #33212 (for reorg scenarios) but could still happen during initial sync.
Fix this more thoroughly by having the node keep track of the last flushed block, and skipping index commits if the current block of the index is not an ancestor of the node's last flushed block (similar to the suggestion by stickies-v in https://github.com/bitcoin/bitcoin/pull/33212#pullrequestreview-31408570890.
Fixes#33208Fixes#34261
ACKs for top commit:
achow101:
ACK e9ed898a0d
sedited:
Re-ACK e9ed898a0d
fjahr:
re-ACK e9ed898a0d
Tree-SHA512: 7f4dc6fb942d6726587eb75dece24c79c679d8630320502aca9fa2d2b03b1d25999cd11255ec20344ebbb8985552747e2554e2557b9d2ad0c75db71652d615ab
The oversized `-dbcache` warning currently switches from a fixed `450 MiB` threshold below `2 GiB` of RAM to `75%` of total RAM at `2 GiB`.
This creates a cliff where a small increase in RAM can raise the warning threshold to about `1536 MiB`.
Apply the `75%` factor only to RAM above a `2 GiB` reserve while keeping `DEFAULT_DB_CACHE` as the minimum threshold.
This removes the cliff: the threshold stays at the default until the percentage term exceeds it, then grows by `0.75 MiB` per additional MiB of RAM.
This also aligns better with the recently merged parallel input prevout fetcher which performs better with slightly lower dbcache memory.
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
In DisconnectBlock(), we can call FlushStateToDisk after updating
the coins but before changing the tip, which is still at the
disconnected block. This means that the ChainStateFlushed
signal would have the wrong block in the locator.
Also remove an outdated comment - the wallet doesn't use
ChainStateFlushed anymore, currently only indexes do.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Otherwise, if the node has an unclean restart,
indexes with state (coinstatsindex) couldn't reorg to the
last flushed tip and would be corrupted.
Also updates documentation of Commit() -
the locator functionality isn't used, so the previous text was wrong:
We must have the best block in our block index after a restart.
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
