faba08b5b4dd5dedb457db18696de6e15839c696 refactor: Remove stray cs_main redundant declaration (MarcoFalke)
fa02591edff0255c5120b5acb2366542a61c251f doc: Export threadsafety.h from sync.h (MarcoFalke)
Pull request description:
Looks like this was forgotten when introducing kernel/cs_main ?
Also, there is a commit to export threadsafety.h from sync.h.
ACKs for top commit:
hebasto:
ACK faba08b5b4dd5dedb457db18696de6e15839c696
Tree-SHA512: 0aa58e7693b6fcd504f9da7339f8baa463a6407f67b27f68002db705f4642321ac3765f16c3d906c925ee24085591b79160a62fa5f4aaf6f2e5dcc788411800d
eeee61065fe165dcce9625f7cc4cfce9e432aafa Use AutoFile and HashVerifier where possible (MarcoFalke)
fa961141f7fc515fbb6fcb9d8417108034b256ce Add HashVerifier (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile` and `HashVerifier`. `CAutoFile` and `CHashVerifier` remain in places where it is not yet possible.
ACKs for top commit:
stickies-v:
ACK eeee61065fe165dcce9625f7cc4cfce9e432aafa
Tree-SHA512: 93786778c309ecfdc1ed43552d24ff9d966954d69a47f66faaa6de24daacd25c651f3f62bde5abbb362700298fb3c04ffbd3207a0dd13d0bd8bff7fd6d07dcf8
282019cd3ddb060db350654e6f855f7ea37e0d34 refactor: add kernel/cs_main.* (fanquake)
Pull request description:
One place to find / include `cs_main`.
No more:
> // Actually declared in validation.cpp; can't include because of circular dependency.
> extern RecursiveMutex cs_main;
Ultimately, no more need to include `validation.h` (which also includes (heavy/boost filled) `txmempool.h`) everywhere for `cs_main`. See #26087 for another example of why that is useful.
ACKs for top commit:
ajtowns:
ACK 282019cd3ddb060db350654e6f855f7ea37e0d34
Tree-SHA512: 142835b794873e7a09c3246d6101843ae81ec0c6295e6873130c98a2abfa5f7282748d0f1a37237a779cc71c3bc0a75d03b20313ef5398c83d4814215cbc8287
04528054fcde61aa00e009dbbe1ac350ca1cf748 [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4ff6ee5057b46bcb8b55abc4422e6f8 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662ce3ab7ba6bbe9813c55369edd6e4c9 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb697aedbe1966ebc2817ab87232a1b59 [miner] allow bypassing TestBlockValidity (glozow)
c0588523083c9c78770b8b19a52a919db56250d9 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1c588488dde653a76853666429d4911 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)
Pull request description:
Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.
ACKs for top commit:
kevkevinpal:
Tested ACK [0452805](04528054fc)
achow101:
ACK 04528054fcde61aa00e009dbbe1ac350ca1cf748
stickies-v:
ACK 04528054f
Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5
55696a0ac30bcfbd555f71cbc8eac23b725f7dcf wallet: remove `mempool_sequence` from `transactionRemovedFromMempool` (w0xlt)
bf19069c53501231a2f3ba59afa067913ec4d3b2 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 55696a0ac3
Tree-SHA512: 621e89230bcb6edfed83e2758601a2b093822fc2dc4e9bfb00487e340f2bc4c5ac3bf6df3ca00b7fe55bb3df15858820f2bf698f403d2e48b915dd9eb47b63e0
47c4b1f52ab8d95d7deef83050bad49d1e3e5990 mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849313ff947f38433b1ac28285a7f7694 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff95eba3793fffaf71a31cc8bfc6f80c9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f7399b6511f9b73b1cef54b6a6ac38a024 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 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
w0xlt:
ACK 47c4b1f52a
glozow:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
Allows us to test BlockAssembler on transactions without signatures or
mature coinbases (which is what PopulateMempool creates). Also means
that `TestBlockValidity()` is not included in the bench timing.
This allows us to both manually manipulate options and grab values from
ArgsManager (i.e. -blockmaxweight and -blockmintxfee config options)
when constructing BlockAssembler::Options. Prior to this change, the
only way to apply the config options is by ctoring BlockAssembler with
no options, which calls DefaultOptions().
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.
b19c4124b3c9a15fe3baf8792c55eb26eca51c0f refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c6025a51d88000caf28121ec00499db build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109281f750909d1feade784c778d8b592 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 b19c4124b3c9a15fe3baf8792c55eb26eca51c0f
Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
38941a703e079709bb465ad1fcde50e11350f8f1 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 38941a703e079709bb465ad1fcde50e11350f8f1 📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
46339d29b10c9fb597af928c21c34945d76bbd22 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f153b84e50191366a6f64f884ed4ddd9 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112ee91923adf38b75491bedeb45f87c80 p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818f60e8297d42b49a919aa82c42821b5 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba211d05869800497d6b518ea1ddd2c718 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f0736919ea4a76f7d45085587625aba p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e293dcd11ca077b7c1c72b06119437faa p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69cf075ac8dff3507bf9151400ed255b7 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 46339d29b10c9fb597af928c21c34945d76bbd22
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b10c9fb597af928c21c34945d76bbd22
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
c8dc0e3eaa9e0f956c5177bcb69632beb0d51770 refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e54402ed248ecf2bdfe54e8283d20fff 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 c8dc0e3eaa9e0f956c5177bcb69632beb0d51770. 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 c8dc0e3eaa9e0f956c5177bcb69632beb0d51770
glozow:
utACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
0582932260e7de4e8aba01d63e7c8a9ddb9c3685 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner)
ca48a4694f73e5be8f971ae482ebc2cce4caef44 rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner)
3449880b499d54bfbcf6caeed52851ce55259ed7 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner)
935c6c4b234bbb0565cda6f58ee298048856acae wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner)
70b35139040a2351c845a1cec1dafd2fbcd16e93 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner)
c051026586fb269584bcba41de8a4a90280f5a7e wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner)
845279132b494f03b84d689c666fdcfad37f5a42 wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner)
088e38d3bbea9694b319bc34e0d2e70d210c38b4 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 0582932260e7de4e8aba01d63e7c8a9ddb9c3685
Sjors:
re-utACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685
aureleoules:
ACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 - minor changes, documentation and updated test since last review
w0xlt:
re-ACK 0582932260
Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
aaaa7bd0ba60aa7114810d4794940882d987c0aa iwyu: Add missing includes (MacroFake)
fa9ebec096ae185576a54aa80bd2a9e57f867ed4 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f988fa7f2ace2d67b1628211b7819df Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86288fa6666307a77c106c8f069ebdbe Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099453bf0882bce4f897f77eee5836e8b Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57cdeb91c8798d5c90ba9cc18085e99fb Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db555f7488c8df96db9fa88b793f897c Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
ryanofsky:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
This is useful for speeding up wallet rescans and is based on an
earlier version from PR #15845 ("wallet: Fast rescan with BIP157 block
filters"), which was never merged.
Co-authored-by: MacroFake <falke.marco@gmail.com>
e133264c5b1f72e94dcb9cebd85cdb523fcf8070 Add test for PSBT input verification (Greg Sanders)
d25699280af1ea45bebc884f63a10da7ea275ef9 Verify PSBT inputs rather than check for fields being empty (Greg Sanders)
Pull request description:
In a few keys spots, PSBT finality is checked by looking for non-empty witness data.
This complicates a couple things:
1) Empty data can be valid in certain cases
2) User may be passed bogus final data by a counterparty during PSBT work happening, and end up with incorrect signatures that they may not be able to check in other contexts if the UTXO doesn't exist yet in chain/mempool, timelocks, etc.
On the whole I think these heavier checks are worth it in case someone is actually assuming the signatures are correct if our API is saying so.
ACKs for top commit:
achow101:
ACK e133264c5b1f72e94dcb9cebd85cdb523fcf8070
Tree-SHA512: 9de4fbb0be1257b081781f5df908fd55666e3acd5c4e36beb3b3f2f5a6aed69ff77068c44cde6127e159e773293fd9ced4c0bb47e693969f337e74dc8af030da
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
Once we received a reconciliation announcement support
message from a peer and it doesn't violate our protocol,
we store the negotiated parameters which will be used
for future reconciliations.
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
bf9597606166323158bbf631137b82d41f39334f doc: add note about snapshot chainstate init (James O'Beirne)
e4d799528696c5ede38c257afaffd367917e0de8 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336d93a2dc88e4a61c49941887766bd72 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c08a00f1f407f1534853a5cddbbc0a23 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8f5971eb3c7b620aa7ad9b437cc515e test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215ed900145bd770525a341ba0ed9c027 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2c16a50d854f6f81428a68aa9180509 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247dad3982d54488bcdb3960470315290 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c2b271cb4683da2fb25fd295557f731 validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d159033106cc595cfa852695610bfe419c989c add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8bc5cdf4368ad55e827a873240535b28 init: add utxo snapshot detection (James O'Beirne)
f9f1735f139b6a1f1c7fea50717ff90dc4ba2bce validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100aaaa25c7558eeed8b5c536da99885f db: add StoragePath to CDBWrapper/CCoinsViewDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf9597606166323158bbf631137b82d41f39334f
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf9597606166323158bbf631137b82d41f39334f. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf9597606166323158bbf631137b82d41f39334f
aureleoules:
ACK bf9597606166323158bbf631137b82d41f39334f
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
bcb0cacac28e98a39dc856c574a0872fe17059e9 reindex, log, test: fixes#21379 (mruddy)
Pull request description:
Fixes#21379.
The blocks/blk?????.dat files are mutated and become increasingly malformed, or corrupt, as a result of running the re-indexing process.
The mutations occur after the re-indexing process has finished, as new blocks are appended, but are a result of a re-indexing process miscalculation that lingers in the block manager's `m_blockfile_info` `nSize` data until node restart.
These additions to the blk files are non-fatal, but also not desirable.
That is, this is a form of data corruption that the reading code is lenient enough to process (it skips the extra bytes), but it adds some scary looking log messages as it encounters them.
The summary of the problem is that the re-index process double counts the size of the serialization header (magic message start bytes [4 bytes] + length [4 bytes] = 8 bytes) while calculating the blk data file size (both values already account for the serialization header's size, hence why it is over accounted).
This bug manifests itself in a few different ways, after re-indexing, when a new block from a peer is processed:
1. If the new block will not fit into the last blk file processed while re-indexing, while remaining under the 128MiB limit, then the blk file is flushed to disk and truncated to a size that is 8 greater than it should be. The truncation adds zero bytes (see `FlatFileSeq::Flush` and `TruncateFile`).
1. If the last blk file processed while re-indexing has logical space for the new block under the 128 MiB limit:
1. If the blk file was not already large enough to hold the new block, then the zeros are, in effect, added by `fseek` when the file is opened for writing. Eight zero bytes are added to the end of the last blk file just before the new block is written. This happens because the write offset is 8 too great due to the miscalculation. The result is 8 zero bytes between the end of the last block and the beginning of the next block's magic + length + block.
1. If the blk file was already large enough to hold the new block, then the current existing file contents remain in the 8 byte gap between the end of the last block and the beginning of the next block's magic + length + block. Commonly, when this occcurs, it is due to the blk file containing blocks that are not connected to the block tree during reindex and are thus left behind by the reindex process and later overwritten when new blocks are added. The orphaned blocks can be valid blocks, but due to the nature of concurrent block download, the parent may not have been retrieved and written by the time the node was previously shutdown.
ACKs for top commit:
LarryRuane:
tested code-review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9
ryanofsky:
Code review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9. This is a disturbing bug with an easy fix which seems well-worth merging.
mzumsande:
ACK bcb0cacac28e98a39dc856c574a0872fe17059e9 (reviewed code and did some testing, I agree that it fixes the bug).
w0xlt:
tACK bcb0cacac2
Tree-SHA512: acc97927ea712916506772550451136b0f1e5404e92df24cc05e405bb09eb6fe7c3011af3dd34a7723c3db17fda657ae85fa314387e43833791e9169c0febe51
fabf1cdb206e368a9433abf99a5ea2762a5ed2c0 Use steady clock for bench logging (MacroFake)
faed342a2338d6a1a26cf977671a736662debae4 scripted-diff: Rename time symbols (MacroFake)
Pull request description:
Instead of using `0.001` and similar constants to "convert" an int64_t to milliseconds, use the type-safe `Ticks<>` helper. Also, use steady clock instead of system clock, since the durations are used for benchmarking.
ACKs for top commit:
fanquake:
ACK fabf1cdb206e368a9433abf99a5ea2762a5ed2c0 - validation bench output still looks sane.
Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
33b12e5df6aa14344dd084e0c6f2d34158fca383 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0bf3b1941bfd18b5ffbd5a81be0e56da test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c1e4e985b1d90eddc135b8dd17049a4 refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f8770974bae4ef3fae64e75ef6dd2d3c2 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v)
Pull request description:
Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in https://github.com/bitcoin/bitcoin/pull/25290 to simplify those signatures and callsites.
The purpose of this PR is to improve readability and maintenance, without behaviour change.
As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR.
ACKs for top commit:
hebasto:
ACK 33b12e5df6aa14344dd084e0c6f2d34158fca383, I have reviewed the code and it looks OK, I agree it can be merged.
glozow:
reACK 33b12e5df6aa14344dd084e0c6f2d34158fca383
Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
0f40d653218789aa176ca2f844e3222d2ad890a3 refactor: remove duplicate code from BlockAssembler (James O'Beirne)
Pull request description:
Found while reminding myself how transactions are chosen for blocks. Take it or leave it!
ACKs for top commit:
glozow:
ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
theStack:
Concept and code-review ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits, and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
