55696a0ac3 wallet: remove `mempool_sequence` from `transactionRemovedFromMempool` (w0xlt)
bf19069c53 wallet: remove `mempool_sequence` from `transactionAddedToMempool` (w0xlt)
Pull request description:
This PR removes `mempool_sequence` from `transactionRemovedFromMempool` and `transactionAddedToMempool`.
`mempool_sequence` is not used in these methods, only in ZMQ notifications.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/26752/commits/55696a0ac30bcfbd555f71cbc8eac23b725f7dcf
Tree-SHA512: 621e89230bcb6edfed83e2758601a2b093822fc2dc4e9bfb00487e340f2bc4c5ac3bf6df3ca00b7fe55bb3df15858820f2bf698f403d2e48b915dd9eb47b63e0
47c4b1f52a mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f739 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)
Pull request description:
Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.
## Unexpected behaviour
This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.
In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.
## Improvements
### Return value instead of out-parameters
This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.
### Observability
There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.
ACKs for top commit:
achow101:
ACK 47c4b1f52a
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/26289/commits/47c4b1f52ab8d95d7deef83050bad49d1e3e5990
glozow:
ACK 47c4b1f52a
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52a
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
b19c4124b3 refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky)
Pull request description:
These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library.
Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense.
This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes.
This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase.
ACKs for top commit:
hebasto:
ACK b19c4124b3
Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
38941a703e refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov)
Pull request description:
This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360:
> why not move it to the right place, that is to `kernel/txmempool_entry.h`?
ACKs for top commit:
MarcoFalke:
review ACK 38941a703e📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)
Pull request description:
Non-trivial changes include:
- Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
- Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
- Don't send `sendtxrcncl` to feeler connections.
ACKs for top commit:
vasild:
ACK 46339d29b1
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b1
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
0582932260 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner)
ca48a4694f rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner)
3449880b49 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner)
935c6c4b23 wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner)
70b3513904 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner)
c051026586 wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner)
845279132b wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner)
088e38d3bb add chain interface methods for using BIP 157 block filters (Sebastian Falbesoner)
Pull request description:
## Description
This PR is another take of using BIP 157 block filters (enabled by `-blockfilterindex=1`) for faster wallet rescans and is a modern revival of #15845. For reviewers new to this topic I can highly recommend to read the corresponding PR review club (https://bitcoincore.reviews/15845).
The basic idea is to skip blocks for deeper inspection (i.e. looking at every single tx for matches) if our block filter doesn't match any of the block's spent or created UTXOs are relevant for our wallet. Note that there can be false-positives (see https://bitcoincore.reviews/15845#l-199 for a PR review club discussion about false-positive rates), but no false-negatives, i.e. it is safe to skip blocks if the filter doesn't match; if the filter *does* match even though there are no wallet-relevant txs in the block, no harm is done, only a little more time is spent extra.
In contrast to #15845, this solution only supports descriptor wallets, which are way more widespread now than back in the time >3 years ago. With that approach, we don't have to ever derive the relevant scriptPubKeys ourselves from keys before populating the filter, and can instead shift the full responsibility to that to the `DescriptorScriptPubKeyMan` which already takes care of that automatically. Compared to legacy wallets, the `IsMine` logic for descriptor wallets is as trivial as checking if a scriptPubKey is included in the ScriptPubKeyMan's set of scriptPubKeys (`m_map_script_pub_keys`): e191fac4f3/src/wallet/scriptpubkeyman.cpp (L1703-L1710)
One of the unaddressed issues of #15845 was that [the filter was only created once outside the loop](https://github.com/bitcoin/bitcoin/pull/15845#discussion_r343265997) and as such didn't take into account possible top-ups that have happened. This is solved here by keeping a state of ranged `DescriptorScriptPubKeyMan`'s descriptor end ranges and check at each iteration whether that range has increased since last time. If yes, we update the filter with all scriptPubKeys that have been added since the last filter update with a range index equal or higher than the last end range. Note that finding new scriptPubKeys could be made more efficient than linearly iterating through the whole `m_script_pub_keys` map (e.g. by introducing a bidirectional map), but this would mean introducing additional complexity and state and it's probably not worth it at this time, considering that the performance gain is already significant.
Output scripts from non-ranged `DescriptorScriptPubKeyMan`s (i.e. ones with a fixed set of output scripts that is never extended) are added only once when the filter is created first.
## Benchmark results
Obviously, the speed-up indirectly correlates with the wallet tx frequency in the scanned range: the more blocks contain wallet-related transactions, the less blocks can be skipped due to block filter detection.
In a [simple benchmark](https://github.com/theStack/bitcoin/blob/fast_rescan_functional_test_benchmark/test/functional/pr25957_benchmark.py), a regtest chain with 1008 blocks (corresponding to 1 week) is mined with 20000 scriptPubKeys contained (25 txs * 800 outputs) each. The blocks each have a weight of ~2500000 WUs and hence are about 62.5% full. A global constant `WALLET_TX_BLOCK_FREQUENCY` defines how often wallet-related txs are included in a block. The created descriptor wallet (default setting of `keypool=1000`, we have 8*1000 = 8000 scriptPubKeys at the start) is backuped via the `backupwallet` RPC before the mining starts and imported via `restorewallet` RPC after. The measured time for taking this import process (which involves a rescan) once with block filters (`-blockfilterindex=1`) and once without block filters (`-blockfilterindex=0`) yield the relevant result numbers for the benchmark.
The following table lists the results, sorted from worst-case (all blocks contain wallte-relevant txs, 0% can be skipped) to best-case (no blocks contain walltet-relevant txs, 100% can be skipped) where the frequencies have been picked arbitrarily:
wallet-related tx frequency; 1 tx per... | ratio of irrelevant blocks | w/o filters | with filters | speed gain
--------------------------------------------|-----------------------------|-------------|--------------|-------------
~ 10 minutes (every block) | 0% | 56.806s | 63.554s | ~0.9x
~ 20 minutes (every 2nd block) | 50% (1/2) | 58.896s | 36.076s | ~1.6x
~ 30 minutes (every 3rd block) | 66.67% (2/3) | 56.781s | 25.430s | ~2.2x
~ 1 hour (every 6th block) | 83.33% (5/6) | 58.193s | 15.786s | ~3.7x
~ 6 hours (every 36th block) | 97.22% (35/36) | 57.500s | 6.935s | ~8.3x
~ 1 day (every 144th block) | 99.31% (143/144) | 68.881s | 6.107s | ~11.3x
(no txs) | 100% | 58.529s | 5.630s | ~10.4x
Since even the (rather unrealistic) worst-case scenario of having wallet-related txs in _every_ block of the rescan range obviously doesn't take significantly longer, I'd argue it's reasonable to always take advantage of block filters if they are available and there's no need to provide an option for the user.
Feedback about the general approach (but also about details like naming, where I struggled a lot) would be greatly appreciated. Thanks fly out to furszy for discussing this subject and patiently answering basic question about descriptor wallets!
ACKs for top commit:
achow101:
ACK 0582932260
Sjors:
re-utACK 0582932260
aureleoules:
ACK 0582932260 - minor changes, documentation and updated test since last review
w0xlt:
re-ACK https://github.com/bitcoin/bitcoin/pull/25957/commits/0582932260e7de4e8aba01d63e7c8a9ddb9c3685
Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
