743abbcbde refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e90 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9 refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698 refactor: inline constant return value of `CDBWrapper::WriteBatch` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a return value when it actually communicates errors through exceptions.
### Solution
This PR removes the constant return values from write methods and inlines `true` at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.
Methods that returned a constant `true` value that now return `void`:
- `CDBWrapper::WriteBatch`, `CDBWrapper::Write`, `CDBWrapper::Erase`
- `TxIndex::DB::WriteTxs`
- `BlockTreeDB::WriteReindexing`, `BlockTreeDB::WriteBatchSync`, `BlockTreeDB::WriteFlag`
- `BlockManager::WriteBlockIndexDB`
### Note
`CCoinsView::BatchWrite` (and transitively `CCoinsViewCache::Flush` & `CCoinsViewCache::Sync`) were intentionally not changed here. While all implementations return `true`, the base `CCoinsView::BatchWrite` returns `false`. Changing this would cause `coins_view` tests to fail with:
> terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared
We can fix that in a follow-up PR.
ACKs for top commit:
achow101:
ACK 743abbcbde
janb84:
ACK 743abbcbde
TheCharlatan:
ACK 743abbcbde
sipa:
ACK 743abbcbde
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
dcb56fd4cb interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb
ryanofsky:
Code review ACK dcb56fd4cb, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb
TheCharlatan:
ACK dcb56fd4cb
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
07a926474b node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
Previously the `bool relay` argument to `BroadcastTransaction()` designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b
kevkevinpal:
ACK [07a9264](07a926474b)
laanwj:
Concept and code review ACK 07a926474b. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
1a1f46c228 refactor/doc: Add blockman param to `GetTransaction` doc comment and reorder out param (Musa Haruna)
Pull request description:
Follow-up to [#27125](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
This PR addresses a minor documentation and style nit mentioned during review:
- Adds the missing `@param[in] blockman` line to the `GetTransaction()` doc comment.
- Moves the output parameter `hashBlock` to the end of both the function
declaration and definition, as suggested in the comment.
ACKs for top commit:
l0rinc:
ACK 1a1f46c228
maflcko:
re-lgtm-ut-cr-rfm-ACK 1a1f46c228
kevkevinpal:
reACK [1a1f46c](1a1f46c228)
Tree-SHA512: 5807a1ae6c383e691e948648dcb1e029620eaff3dcdff73d88c6dc268a7af5559a30c491b72f038b3f7e812e1845f4f063b49bd3671edfac1bb3a170c84be4f5
1a7fb5eeee fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480d9b fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a435 fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e034 fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5eeee🐤
polespinasa:
re ACK 1a7fb5eeee
willcl-ark:
ACK 1a7fb5eeee
janb84:
re ACK 1a7fb5eeee
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
0465574c12 test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e7 test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23 Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b4 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c12
TheCharlatan:
ACK 0465574c12
furszy:
Tested ACK 0465574c12.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
Previously the `bool relay` argument to `BroadcastTransaction()`
designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend
with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Block template fees are calculated by looping over new_tmpl->vTxFees
and return (early) once the fee_threshold is exceeded.
This left an edge case when the mempool is empty, which this commit
fixes and adds a test for. It does so by using std::accumulate instead
of manual loops.
Also update interface_ipc.py to account for the new behavior.
Co-authored-by: Raimo33 <claudio.raimondi@protonmail.com>
b807dfcdc5 miner: fix `addPackageTxs` unsigned integer overflow (ismaelsadeeq)
Pull request description:
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions equivalent to `MAX_CONSECUTIVE_FAILURES`, the loop never breaks until all mempool transactions are visited.
See https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282
The fix avoids the overflow by using addition instead adding `BLOCK_FULL_ENOUGH_WEIGHT_DELTA` to the block weight and comparing it with `m_options.nBlockMaxWeight`.
Another alternative that preserves the same structure is to use `static_cast`. See c9530cf35d.
This fix can be tested by cherry-picking the commits from #33421 without the static cast fix and running:
```bash
echo "AQAAAAAAA
AAnJycnAAAAAAAAAAAAAAAAAA" | base64 --decode > miner.crash
FUZZ=block_template_cache ./build_fuzz/bin/fuzz miner.crash
```
---
This is part of a larger inconsistency in how size/weight is represented in the codebase. It may be worth defining a dedicated type for size/weight.
ACKs for top commit:
glozow:
nice, utACK b807dfcdc5
furszy:
Code ACK b807dfcdc5
Tree-SHA512: c1d2f7e500f9b0624a4c22a146921a1644017065e6c94d0c5027486392321f5de26c61751a24765e025e45b34c535adfd6d0e2ac809dea6846b99f37d13043c9
Oversized allocations can cause out-of-memory errors or [heavy swapping](https://github.com/getumbrel/umbrel-os/issues/64#issuecomment-663637321), [grinding the system to a halt](https://x.com/murchandamus/status/1964432335849607224).
`LogOversizedDbCache()` now emits a startup warning if the configured `-dbcache` exceeds a cap derived from system RAM, using the same parsing/clamping as cache sizing via CalculateDbCacheBytes(). This isn't meant as a recommended setting, rather a likely upper limit.
Note that we're not modifying the set value, just issuing a warning.
Also note that the 75% calculation is rounded for the last two numbers since we have to divide first before multiplying, otherwise we wouldn't stay inside size_t on 32-bit systems - and this was simpler than casting back and forth.
We could have chosen the remaining free memory for the warning (e.g. warn if free memory is less than 1 GiB), but this is just a heuristic, we assumed that on systems with a lot of memory, other processes are also running, while memory constrained ones run only Core.
If total RAM < 2 GiB, cap is `DEFAULT_DB_CACHE` (`450 MiB`), otherwise it's 75% of total RAM.
The threshold is chosen to be close to values commonly used in [raspiblitz](https://github.com/raspiblitz/raspiblitz/blob/dev/home.admin/_provision.setup.sh#L98-L115) for common setups:
| Total RAM | `dbcache` (MiB) | raspiblitz % | proposed cap (MiB) |
|----------:|----------------:|-------------:|-------------------:|
| 1 GiB | 512 | 50.0% | 450* |
| 2 GiB | 1536 | 75.0% | 1536 |
| 4 GiB | 2560 | 62.5% | 3072 |
| 8 GiB | 4096 | 50.0% | 6144 |
| 16 GiB | 4096 | 25.0% | 12288 |
| 32 GiB | 4096 | 12.5% | 24576 |
[Umbrel issues](https://github.com/getumbrel/umbrel-os/issues/64#issuecomment-663816367) also mention 75% being the upper limit.
Starting `bitcoind` on an 8 GiB rpi4b with a dbcache of 7 GiB:
> ./build/bin/bitcoind -dbcache=7000
warns now as follows:
```
2025-09-07T17:24:29Z [warning] A 7000 MiB dbcache may be too large for a system memory of only 7800 MiB.
2025-09-07T17:24:29Z Cache configuration:
2025-09-07T17:24:29Z * Using 2.0 MiB for block index database
2025-09-07T17:24:29Z * Using 8.0 MiB for chain state database
2025-09-07T17:24:29Z * Using 6990.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
Besides the [godbolt](https://godbolt.org/z/EPsaE3xTj) reproducers for the new total memory method, we also tested the warnings manually on:
- [x] Apple M4 Max, macOS 15.6.1
- [x] Intel Core i9-9900K, Ubuntu 24.04.2 LTS
- [x] Raspberry Pi 4 Model B, Armbian Linux 6.12.22-current-bcm2711
- [x] Intel Xeon x64, Windows 11 Home Version 24H2, OS Build 26100.4351
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
ba84a25dee [doc] update mempool-replacements.md for incremental relay feerate change (glozow)
18720bc5d5 [doc] release note for min feerate changes (glozow)
6da5de58ca [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB (glozow)
2e515d2897 [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit (glozow)
457cfb61b5 [prep/util] help MockMempoolMinFee handle more precise feerates (glozow)
3eab8b7240 [prep/test] replace magic number 1000 with respective feerate vars (glozow)
5f2df0ef78 [miner] lower default -blockmintxfee to 1sat/kvB (glozow)
d6213d6aa1 [doc] assert that default min relay feerate and incremental are the same (glozow)
1fbee5d7b6 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee (glozow)
72dc18467d [test] RBF rule 4 for various incrementalrelayfee settings (glozow)
85f498893f [test] check bypass of minrelay for various minrelaytxfee settings (glozow)
e5f896bb1f [test] check miner doesn't select 0fee transactions (glozow)
Pull request description:
ML post for discussion about the general concept, how this impacts the wider ecosystem, philosophy about minimum feerates, etc: https://delvingbitcoin.org/t/changing-the-minimum-relay-feerate/1886
This PR is inspired by #13922 and #32959 to lower the minimum relay feerate in response to bitcoin's exchange rate changes in the last ~10 years. It lowers the default `-minrelaytxfee` and `-incrementalrelayfee`, and knocks `-blockmintxfee` down to the minimum nonzero setting. Also adds some tests for the settings and pulls in #32750.
The minimum relay feerate is a DoS protection rule, representing a price on the network bandwidth used to relay transactions that have no PoW. While relay nodes don't all collect fees, the assumption is that if nodes on the network use their resources to relay this transaction, it will reach a miner and the attacker's money will be spent once it is mined. The incremental relay feerate is similar: it's used to price the relay of replacement transactions (the additional fees need to cover the new transactions at this feerate) and evicted transactions (following a trim, the new mempool minimum feerate is the package feerate of what was removed + incremental).
Also note that many nodes on the network have elected to relay/mine lower feerate transactions. Miners (some say up to 85%) are choosing to mine these low feerate transactions instead of leaving block space unfilled, but these blocks have extremely poor compact block reconstruction rates with nodes that rejected or didn't hear about those transactions earlier.
- https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155627414
- https://x.com/caesrcd/status/1947022514267230302
- https://mempool.space/block/00000000000000000001305770e0aa279dcd8ba8be18c3d5cf736a26f77e06fd
- https://mempool.space/block/00000000000000000001b491649ec030aa8e003e1f4f9d3b24bb99ba16f91e97
- https://x.com/mononautical/status/1949452586391855121
While it wouldn't make sense to loosen DoS restrictions recklessly in response to these events, I think the current price is higher than necessary, and this motivates us changing the default soon. Since the minimum relay feerate defines an amount as too small based on what it costs the attacker, it makes sense to consider BTC's conversion rate to what resources you can buy in the "real world."
Going off of [this comment](https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3095260286) and [this comment](https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3142444090)
- Let's say an attacker wants to use/exhaust the network's bandwidth, and has the choice between renting resources from a commercial provider and getting the network to "spam" itself it by sending unconfirmed transactions. We'd like the latter to be more expensive than the former.
- The bandwidth for relaying a transaction across the network is roughly its serialized size (plus relay overhead) x number of nodes. A 1000vB transaction is 1000-4000B serialized. With 100k nodes, that's 0.1-0.4GB
- If the going rate for ec2 bandwidth is 10c/GB, that's like 1-4c per kvB of transaction data
- Then a 1000vB transaction should pay at least 4c
- $0.04 USD is 40 satoshis at 100k USD/BTC
- Baking in some margin for changes in USD/BTC conversion rate, number of nodes (and thus bandwidth), and commercial service costs, I think 50-100 satoshis is on the conservative end but in the right ballpark
- At least 97% of the recent sub-1sat/vB transactions would be accepted with a new threshold of 0.1sat/vB: https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156213089
List of feerates that are changed and why:
- min relay feerate: significant conversion rate changes, see above
- incremental relay feerate: should follow min relay feerate, see above
- block minimum feerate: shouldn’t be above min relay feerate, otherwise the node accepts transactions it will never mine. I've knocked it down to the bare minimum of 1sat/kvB. Now that we no longer have coin age priority (removed in v0.15), I think we can leave it to the `CheckFeeRate` policy rule to enforce a minimum entry price, and the block assembly code should just fill up the block with whatever it finds in mempool.
List of feerates that are not changed and why:
- dust feerate: this feerate cannot be changed as flexibly as the minrelay feerate. A much longer record of low feerate transactions being mined is needed to motivate a decrease there.
- maxfeerate (RPC, wallet): I think the conversion rate is relevant as well, but out of scope for this PR
- minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represent miner policy (e.g. #9519). Also, the fee estimator itself doesn't have support for sub-1sat/vB yet.
- all wallet feerates (mintxfee, fallbackfee, discardfee, consolidatefeerate, WALLET_INCREMENTAL_RELAY_FEE, etc.): should be done later. Our standard procedure is to do wallet changes at least 1 release after policy changes.
ACKs for top commit:
achow101:
ACK ba84a25dee
gmaxwell:
ACK ba84a25dee
jsarenik:
Tested ACK ba84a25dee
darosior:
ACK ba84a25dee
ajtowns:
ACK ba84a25dee
davidgumberg:
crACK ba84a25dee
w0xlt:
ACK ba84a25dee
caesrcd:
reACK ba84a25dee
ismaelsadeeq:
re-ACK ba84a25dee
Tree-SHA512: b4c35e8b506b1184db466551a7e2e48bb1e535972a8dbcaa145ce3a8bfdcc70a8807dc129460f129a9d31024174d34077154a387c32f1a3e6831f6fa5e9c399e
2b00030af8 interfaces, chain, refactor: Remove inaccurate getActiveChainLocator (pablomartin4btc)
110a0f405c interfaces, chain, refactor: Remove unused getTipLocator (pablomartin4btc)
Pull request description:
Remove `Chain::getTipLocator`, `Chain::GetLocator()`, and `Chain::getActiveChainLocator`:
- `Chain::getTipLocator` is no longer used.
- `Chain::GetLocator`, replaced its call by `GetLocator()`, which uses `LocatorEntries`, avoiding direct access to the chain itself (change suggested by l0rinc while reviewing this PR to maintain consistency with the overall refactoring).
- `Chain::getActiveChainLocator`, whose name was misleading, has functionality redundant with Chain::findBlock.
- Additionally, the comment for getActiveChainLocator became inaccurate following changes in commit ed470940cd (from PR #25717).
This is a [follow-up](https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-3151665095) to #29652.
ACKs for top commit:
achow101:
ACK 2b00030af8
furszy:
ACK 2b00030af8
stickies-v:
ACK 2b00030af8
w0xlt:
ACK 2b00030af8
Tree-SHA512: b12ba6a15feeaeec692d69204a6e155e3af43edfac25597dabf14cacca1e4a2152574816e58dc544f39043c5721f5e707acf544f4541d3b9c0f8c0c40069215e
Did both in this commit, since the return value of `WriteReindexing` was ignored anyway - which existed only because of the constant `Erase` being called
`WriteBatch` can only ever return `true` - its errors are handled by throwing a `throw dbwrapper_error` instead.
The boolean return value is quite confusing, especially since it's symmetric with `CDBWrapper::Read`, which catches the exceptions and returns a boolean instead.
We're removing the constant return value and inlining `true` for its usages.
The getActiveChainLocator method name was misleading, and its functionality
duplicated `Chain::findBlock`. This commit removes the method and replaces
all its usages with direct `Chain::findBlock` calls.
Additionally, the comment of getActiveChainLocator has been outdated since
commit ed47094 from #25717.
Finally, in CWallet::ScanForWalletTransactions, the findBlock calls are now
unified into a single call at the start of the function.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Matias Furszyfer <mfurszy@protonmail.com>
Also removed CChain::GetLocator() and replaced its call
with GetLocator() which uses LocatorEntries instead.
Co-authored-by: ryanofsky <ryan@ofsky.org>
Co-authored-by: l0rinc <l0rinc@users.noreply.github.com>
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
c0642e558a [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448923 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6704 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f1 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01696 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3 [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a
marcofleon:
Nice, ACK c0642e558a
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
-BEGIN VERIFY SCRIPT-
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp
sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp
-END VERIFY SCRIPT-
This introduces an invariant that TxOrphanageImpl never holds more than one
announcement with m_reconsider=true for a given wtxid. This avoids duplicate
work, both in the caller might otherwise reconsider the same transaction multiple
times before it is ready, and internally in AddChildrenToWorkSet, which might
otherwise iterate over all announcements multiple times.
Before this, if we had two (or more) same work tip candidates and restarted our node,
it could be the case that the block set as tip after bootstrap didn't match the one
before stopping. That's because the work and `nSequenceId` of both block will be the same
(the latter is only kept in memory), so the active chain after restart would have depended
on what tip candidate was loaded first.
This makes sure that we are consistent over reboots.
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
249889bee6 orphanage: avoid vtx iteration when no orphans (furszy)
41ad2be434 mempool: Avoid expensive loop in `removeForBlock` during IBD (Lőrinc)
Pull request description:
During Initial Block Download, the mempool is usually empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with `40 bytes * vtx.size()`, even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this change introduces a minor performance & memory optimization by only executing the loop if any of the affected mempool maps have any contents. The second commit is cherry-picked from there since it's related to this change as well.
ACKs for top commit:
optout21:
ACK 249889bee6
glozow:
ACK 249889bee6
ismaelsadeeq:
reACK 249889bee6
Tree-SHA512: 80d06ff1515164529cdc3ad21db3041bb5b2a1d4b72ba9e6884cdf40c5f1477fee7479944b8bca32a6f0bf27c4e5501fccd085f6041a2dbb101438629cfb9e4b
Mechanical refactor of the low-level "xor" wording to signal the intent instead of the implementation used.
The renames are ordered by heaviest-hitting substitutions first, and were constructed such that after each replacement the code is still compilable.
-BEGIN VERIFY SCRIPT-
sed -i \
-e 's/\bGetObfuscateKey\b/GetObfuscation/g' \
-e 's/\bxor_key\b/obfuscation/g' \
-e 's/\bxor_pat\b/obfuscation/g' \
-e 's/\bm_xor_key\b/m_obfuscation/g' \
-e 's/\bm_xor\b/m_obfuscation/g' \
-e 's/\bobfuscate_key\b/m_obfuscation/g' \
-e 's/\bOBFUSCATE_KEY_KEY\b/OBFUSCATION_KEY_KEY/g' \
-e 's/\bSetXor(/SetObfuscation(/g' \
-e 's/\bdata_xor\b/obfuscation/g' \
-e 's/\bCreateObfuscateKey\b/CreateObfuscation/g' \
-e 's/\bobfuscate key\b/obfuscation key/g' \
$(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
Since 31 byte xor-keys are not used in the codebase, using the common size (8 bytes) makes the benchmarks more realistic.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
