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.
bf95976061 doc: add note about snapshot chainstate init (James O'Beirne)
e4d7995286 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c0 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8 test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d1590331 add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8b init: add utxo snapshot detection (James O'Beirne)
f9f1735f13 validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100 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 bf95976061
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf95976061. 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 bf95976061
aureleoules:
ACK bf95976061
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
bcb0cacac2 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 bcb0cacac2
ryanofsky:
Code review ACK bcb0cacac2. This is a disturbing bug with an easy fix which seems well-worth merging.
mzumsande:
ACK bcb0cacac2 (reviewed code and did some testing, I agree that it fixes the bug).
w0xlt:
tACK bcb0cacac2
Tree-SHA512: acc97927ea712916506772550451136b0f1e5404e92df24cc05e405bb09eb6fe7c3011af3dd34a7723c3db17fda657ae85fa314387e43833791e9169c0febe51
fabf1cdb20 Use steady clock for bench logging (MacroFake)
faed342a23 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 fabf1cdb20 - validation bench output still looks sane.
Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
33b12e5df6 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0b test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f87 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 33b12e5df6, I have reviewed the code and it looks OK, I agree it can be merged.
glozow:
reACK 33b12e5df6
Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
0f40d65321 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 0f40d65321
theStack:
Concept and code-review ACK 0f40d65321
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.
If we call FlushBlockFile() without having intitialized the block index
with LoadBlockIndexDB(), we may be indexing into an empty vector.
Specifically this is an issue when we call MaybeRebalanceCaches() during
chainstate init before the block index has been loaded, which calls
FlushBlockFile().
Also add an assert to avoid undefined behavior.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Add functionality for activating a snapshot-based chainstate if one is
detected on-disk.
Also cautiously initialize chainstate cache usages so that we don't
somehow blow past our cache allowances during initialization, then
rebalance at the end of init.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This changes the snapshot's leveldb chainstate dir name from
`chainstate_[blockhash]` to `chainstate_snapshot`. This simplifies
later logic that loads snapshot data, and enforces the limitation
of a single snapshot at any given time.
Since we still need to persis the blockhash of the base block, we
write that out to a file (`chainstate_snapshot/base_blockhash`) for
later use during initialization, so that we can reinitialize the
snapshot chainstate.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
fa4c59d65b Move blockstorage option logging to LoadChainstate() (MacroFake)
fa3358b668 Move validation option logging to LoadChainstate() (MacroFake)
Pull request description:
This would allow libbitcoinkernel users to see the options logged as well. Currently they would only be logged for bitcoind. Behavior change suggested in the refactoring pull https://github.com/bitcoin/bitcoin/pull/25704#discussion_r956166460
ACKs for top commit:
ryanofsky:
Code review ACK fa4c59d65b. Only change since last review is moving pruning logprints out of `AppInitParameterInteraction` as suggested
jonatack:
Review ACK fa4c59d65b
Tree-SHA512: f27508ca06a78ef162f002d556cf830df374fe95fd4f10bf22c24b6b48276ce49f52f82ffedc43596c872ddcf08321ca03651495fd3abde16254cb8afab39d33
fa875349e2 Fix iwyu (MacroFake)
faad673716 Fix issues when calling std::move(const&) (MacroFake)
Pull request description:
Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways:
* Remove the `const`, or
* Remove the `std::move`
ACKs for top commit:
ryanofsky:
Code review ACK fa875349e2. Looks good. Good for univalue to support c++11 move optimizations
Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
This makes a number of changes:
- Get rid of the verification_progress argument in the node interface
NotifyHeaderTip (it was always 0.0).
- Instead of passing a CBlockIndex* in the UI interface's NotifyHeaderTip,
send separate height, timestamp fields. This is becuase in headers presync,
no actual CBlockIndex object is available.
- Add a bool presync argument to both of the above, to identify signals
pertaining to the first headers sync phase.
This introduces an insignificant performance penalty, as it means locator
construction needs to use the skiplist-based CBlockIndex::GetAncestor()
function instead of the lookup-based CChain, but avoids the need for
callers to have access to a relevant CChain object.
1dc03dda05 [doc] remove non-signaling mentions of BIP125 (glozow)
32024d40f0 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow)
Pull request description:
We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy.
We should not use "BIP125" as synonymous with our RBF policy because:
- Our RBF policy is different from what is specified in BIP125, for example:
- the BIP does not mention our rule about the replacement feerate being higher (our Rule 6)
- the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380
- the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md
- the signaling policy is configurable, see #25353
- Our RBF policy may change further
- We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498
- See comments from people who are not me recently:
- https://github.com/bitcoin/bitcoin/pull/25038#discussion_r909507429
- https://github.com/bitcoin/bitcoin/pull/25575#issuecomment-1179519204
This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because:
- It is succint.
- It has already been widely marketed as BIP125 opt-in signaling.
- Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive.
- If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from.
Alternatives:
- Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6).
- Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.
- Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.
ACKs for top commit:
darosior:
ACK 1dc03dda05
ariard:
ACK 1dc03dda
t-bast:
ACK 1dc03dda05
Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
eeee5ada23 Make adjusted time type safe (MacroFake)
fa3be799fe Add time helpers (MacroFake)
Pull request description:
This makes follow-ups easier to review. Also, it makes sense by itself.
ACKs for top commit:
ryanofsky:
Code review ACK eeee5ada23. Confirmed type changes and equivalent code changes only.
Tree-SHA512: 51bf1ae5428552177286113babdd49e82459d6c710a07b6e80a0a045d373cf51045ee010461aba98e0151d8d71b9b3b5f8f73e302d46ba4558e0b55201f99e9f
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
4bedfd702a refactor: remove unneeded temporaries in node/interfaces, simplify code (Jon Atack)
b27ba169eb refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public (Jon Atack)
Pull request description:
- Make all `NodeImpl`, `ChainImpl` and `ExternalSignerImpl` class members `public` (and document why), to be consistent in all the `*Impl` classes in `src/node/interfaces.cpp` and `src/wallet/interfaces.cpp` and to help future reviewers and contributors.
- Remove unneeded temporaries in `NodeImpl` and `ChainImpl` methods in `src/node/interfaces.cpp` and simplify, to make the code easier to read and understand and to improve performance by avoiding unnecessary move operations.
ACKs for top commit:
ryanofsky:
Code review ACK 4bedfd702a. Changes since last review, applying suggested style & simplifiying first commit. Also avoiding another lock in second commit.
Tree-SHA512: 112f7cad5e2838c94c5b79d61328f42fe75fdb97f401ab49eccf696fc2c6a8a0c0ee55ec974c0602acf7423f78bb82e90eb8a0cc531e1d3347f73b7c83685504
- make the code easier to read and understand
- improve performance by avoiding unnecessary move operations
- the cleaner, simpler, and easier to read the code is, the
better chance the compiler has at implementing it well
as the classes themselves are private, and to be consistent within all the
*Impl classes in src/node/interfaces.cpp and src/wallet/interfaces.cpp
following this order:
public:
// ... virtual methods ...
// ... nonvirtual helper methods ...
// ... data members ...
and add documentation in src/node/interfaces.cpp and src/wallet/interfaces.cpp
to help future reviewers and contributors.
dd065dae9f refactor: Make mapBlocksUnknownParent local, and rename it (Hennadii Stepanov)
Pull request description:
This PR is a second attempt at #19594. This PR has two motivations:
- Improve code hygiene by eliminating a global variable, `mapBlocksUnknownParent`
- Fix fuzz test OOM when running too long ([see #19594 comment](https://github.com/bitcoin/bitcoin/pull/19594#issuecomment-958801638))
A minor added advantage is to release `mapBlocksUnknownParent` memory when the reindexing phase is done. The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.
This PR helps our efforts to reduce the use of global variables. This variable isn't just global, it's hidden inside a function (it looks like a local variable but has the `static` attribute).
This global variable exists because the `-reindex` processing code calls `LoadExternalBlockFile()` multiple times (once for each block file), but that function must preserve some state between calls (the `mapBlocksUnknownParent` map). This PR fixes this by allocating this map as a local variable in the caller's scope and passing it in on each call. When reindexing completes, the map goes out of scope and is deallocated.
I tested this manually by reindexing on mainnet and signet. Also, the existing `feature_reindex.py` functional test passes.
ACKs for top commit:
mzumsande:
re-ACK dd065dae9f
theStack:
re-ACK dd065dae9f
shaavan:
reACK dd065dae9f
Tree-SHA512: 9cd20e44d2fa1096dd405bc107bc065ea8f904f5b3f63080341b08d8cf57b790df565f58815c2f331377d044d5306708b4bf6bdfc5ef8d0ed85d8e97d744732c
fad3c5826e refactor: Fix iwyu on node/chainstate (MacroFake)
Pull request description:
Fix the CI warning on master: https://cirrus-ci.com/task/5398182703136768?logs=ci#L7020
ACKs for top commit:
fanquake:
ACK fad3c5826e - could do chain.h
Tree-SHA512: 94f6ea0b3d9667863a4217b65bd1b9e07c65bdb566378faf0727bae5eb38d2d527ecae0c39efdda740b7ab7c8269141437ffbcb470cca7d559f09b8ee132d101
fa32b1bbfd refactor: Use chainman() helper consistently in ChainImpl (MacroFake)
Pull request description:
Doing anything else will just lead to more verbose and inconsistent code.
ACKs for top commit:
fanquake:
ACK fa32b1bbfd - all instances of `Assert(m_node.chainman)` in node/interfaces replaced with `chainman()`, which is the same thing.
shaavan:
Code Review ACK fa32b1bbfd
Tree-SHA512: a417680f79c150e4431aa89bc9db79fdf2dd409419081eb243194837b4ab8d16434165393f39a157473802753843e8c5314ad05c569b4e9221ce29a9fd1cefb8
