This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
I strongly recommend reviewing with the following git-diff flags:
--color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change
6c23c41561 refactor: Rewrite AddToBlockIndex with try_emplace (Carl Dong)
c05cf7aa1e style: Modernize range-based loops over m_block_index (Carl Dong)
c2a1655799 style-only: Use using instead of typedef for BlockMap (Carl Dong)
dd79dad175 refactor: Rewrite InsertBlockIndex with try_emplace (Carl Dong)
531dce0347 tests: Remove now-unnecessary manual Unload's (Carl Dong)
bec86ae326 blockstorage: Make m_block_index own CBlockIndex's (Carl Dong)
Pull request description:
Part of: #24303
Split off from: #22564
```
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
```
The second commit demonstrates how this makes calls to `Unload()` to satisfy the address sanitizer unnecessary.
ACKs for top commit:
ajtowns:
ACK 6c23c41561
MarcoFalke:
re-ACK 6c23c41561🎨
Tree-SHA512: 81b2b5119be27cc0f8a9457b11da60cc60930315d2a5be36be89fe253d32073ffe622348ff153114b9b3212197bddbc791810913a43811b33cc58e7162bd105b
0eea83a85e scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505db net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85e
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85e code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
A change to BlockManager::LookupBlockIndex:
- Previously, it was a const member function returning a non-const CBlockIndex*
- Now, there's are const and non-const versions of
BlockManager::LookupBlockIndex returning a CBlockIndex with the same
const-ness as the member function:
(e.g. const CBlockIndex* LookupBlockIndex(...) const)
See next commit for some weirdness that this eliminates.
The range based for-loops are modernize (using auto + destructuring) in
a future commit.
fa5d2e678c Remove unused char serialize (MarcoFalke)
fa24493d63 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217 span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c
sipa:
re-utACK fa5d2e678c
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
6ea5682784 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba8 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00577 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced28 Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)
Pull request description:
Issues:
- `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step:
- `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`.
This pull:
- adds thread safety lock annotations for the functions listed above
- guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main
How to review and test:
- debug build with clang and verify there are no `-Wthread-safety-analysis` warnings
- review the code to verify each annotation or lock is necessary and sensible, or if any are missing
- look for whether taking a lock can be replaced by a lock annotation instead
- for more information about Clang thread safety analysis, see
- https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization
Mitigates/potentially closes #17161.
ACKs for top commit:
laanwj:
Code review ACK 6ea5682784
Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
Mutex cs_main is already held by the caller of WriteUndoDataForBlock().
This change is needed to require CBlockIndex::GetUndoPos() to hold
cs_main and CBlockIndex::nStatus to be guarded by cs_main in the
following commits without adding 2 unnecessary cs_main locks to
WriteUndoDataForBlock().
e5b6aef612 Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef612
MarcoFalke:
Concept ACK e5b6aef612🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
The new helper function, BlockManager::WriteBlockIndexDB(),
has a thread safety lock annotation in its declaration but is
missing the corresponding run-time lock assertion in its definition.
Per doc/developer-notes.md: "Combine annotations in function
declarations with run-time asserts in function definitions."
fa68a6c2fc scripted-diff: Rename touched member variables (MarcoFalke)
facd3df21f Make blockstorage globals private members of BlockManager (MarcoFalke)
faa8c2d7d7 doc: Clarify nPruneAfterHeight for signet (MarcoFalke)
fad381b2f8 test: Load genesis block to allow flush (MarcoFalke)
fab262174b Move blockstorage-related unload to BlockManager::Unload (MarcoFalke)
fa467f3913 move-only: Create WriteBlockIndexDB helper (MarcoFalke)
fa88cfd3f9 Move functions to BlockManager (MarcoFalke)
Pull request description:
Globals aren't too nice because they hide dependencies, also they make testing harder.
Fix that by removing some.
ACKs for top commit:
Sjors:
ACK fa68a6c2fc
ryanofsky:
Code review ACK fa68a6c2fc. Nice changes!
Tree-SHA512: 6abc5929a5e43a05e238276721d46a64a44f23dca18c2caa9775437a32351d6815d88b88757254686421531d0df13861bbd3a202e13a3192798d87a96abef65d
e3544c864e init: Use clang-tidy named args syntax (Carl Dong)
3401630417 style-only: Rename *Chainstate return values (Carl Dong)
1dd582782d docs: Make LoadChainstate comment more accurate (Carl Dong)
6b83576388 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong)
Pull request description:
There are 2 proposed fixups in discussions in #23280 which I have not implemented:
1. An overhaul to return types and an option type for the two `*Chainstate` functions: https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-984149564
- The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR.
2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: https://github.com/bitcoin/bitcoin/pull/23280#discussion_r765051533
- I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct.
If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one!
ACKs for top commit:
ryanofsky:
Code review ACK e3544c864e
MarcoFalke:
ACK e3544c864e🐸
Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904
This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
BlockManager::Unload
* Only unit tests call Unload directly
fa996c58e8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d1 Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560 style: Remove unused whitespace (MarcoFalke)
Pull request description:
A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.
Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716
ACKs for top commit:
shaavan:
reACK fa996c58e8
vasild:
ACK fa996c58e8
Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
The helper was moved in commit b026e318c3,
which also mentioned that it could be moved to CChainState. So do that,
as the functionality is not block-storage related.
This also allows to drop one function argument.
7f15eff2dd style-only: Remove redundant scope in *Chainstate (Carl Dong)
89bec827fd Collapse the 2 cs_main locks in LoadChainstate (Carl Dong)
3b1584b794 Remove all #include // for * comments (Carl Dong)
9a5a5a3d08 test/setup: Use LoadChainstate (Carl Dong)
c541da0d62 node/chainstate: Add options for in-memory DBs (Carl Dong)
ceb9790341 node/caches: Remove intermediate variables (Carl Dong)
ac4bf138b8 node/caches: Extract cache calculation logic (Carl Dong)
15f2e33bb3 validation: VerifyDB only needs Consensus::Params (Carl Dong)
4da9c076d1 node/chainstate: Decouple from ShutdownRequested (Carl Dong)
05441c2dc5 node/chainstate: Decouple from GetTime (Carl Dong)
2414ebc18b init: Delay RPC block notif until warmup finished (Carl Dong)
8d466a8504 Move -checkblocks LogPrintf to AppInitMain (Carl Dong)
aad8d59789 node/chainstate: Reduce coupling of LogPrintf (Carl Dong)
b345979a2b node/chainstate: Decouple from concept of uiInterface (Carl Dong)
ca7c0b934d Split off VerifyLoadedChainstate (Carl Dong)
adf4912d77 node/chainstate: Remove do/while loop (Carl Dong)
975235ca0a Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Carl Dong)
8715658983 Move mempool nullptr Assert out of LoadChainstate (Carl Dong)
9162a4f93e node/chainstate: Decouple from concept of NodeContext (Carl Dong)
c7a5c46e6f node/chainstate: Decouple from ArgsManager (Carl Dong)
ae9121f958 node/chainstate: Decouple from stringy errors (Carl Dong)
cbac28b72f node/chainstate: Decouple from GetTimeMillis (Carl Dong)
cb64af9635 node: Extract chainstate loading sequence (Carl Dong)
Pull request description:
This PR:
1. Coalesce the Chainstate loading sequence between `AppInitMain` and `*TestingSetup` (which makes it more tested)
2. Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.
Code-wise, this PR:
1. Extracts `AppInitMain`'s Chainstate loading sequence into a `::LoadChainstateSequence` function
2. Makes this `::LoadChainstateSequence` function reusable by
1. Decoupling it from various concepts (`ArgsManager`, `uiInterface`, etc)
2. Making it report errors using an `enum` rather than by setting a `bilingual_str`
3. Makes `*TestingSetup` use this new `::LoadChainstateSequence`
Reviewers: Aside from commentary, I've also included `git diff` flags of interest in the commit messages which I hope will aid review!
ACKs for top commit:
ryanofsky:
Code review ACK 7f15eff2dd. Thanks for updates!
MarcoFalke:
review ACK 7f15eff2dd💳
Tree-SHA512: fb9a6cbd1c511a52b477c62a5e68e53a8be5dec2fff0e44a279966afb91efbab44bf1fe7c6b1519f8464ecc25f42dd4bae8e1efbf55ee91fc90fa0b92e3a83e2
