This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Can be reviewed with the git option
--color-moved=dimmed-zebra
Concept ACK.
The include sorting is a bit off in some files.
Concept ACK
Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
That's great! :)
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Also, add missing { } for style.
Can be reviewed with `--word-diff-regex=.`force pushed to clang-format the include sorting
Concept ACK. This paves the way to at some point make lock-splitting BlockManager away from cs_main a little bit more straightforward, at least syntactically.
The doc nicely explains why the directory exists and it is
irrelevant when it was introduced. Even if it was relevant,
it could be trivially found out via `git log ./src/node/ | tail`
without visiting GitHub
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
nit, could update doc/productivity.md to mention --color-moved-ws=ignore-all-space.
nit, could update doc/productivity.md to mention --color-moved-ws=ignore-all-space.
Good suggestion. Happy to review another pr, to keep this one focused on ./src/ changes only.
26 | static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
27 |
28 | +/** Functions for disk access for blocks */
29 | +bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
30 | +bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
31 | +bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
https://github.com/bitcoin/bitcoin/pull/21575/commits/fa0c7d9ad24d3c9515d3f9c136af4071cbd79055
It looks like this is unused outside of the src/node/blockstorage.cpp unit, so you could make it internal to that if you wanted to.
I'll keep this one public for symmetry with the other read* functions
ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f (jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto)
Good change, mostly just moves. Though I think the last commit should maybe be amended or reviewed in its own PR. @MarcoFalke I assume that the circular deps introduced here are thought to be temporary?
Built locally and ran unittest suite.
faf843c07f refactor: Move load block thread into ChainstateManager
Would be nice to document the new m_load_block thread.
fa413f07a1 move-only: Move ThreadImport to blockstorage
fa91b2b2b3 move-only: Move AbortNode to shutdown Seems unrelated, but maybe isn't? ACK anyway.
fa0c7d9ad2 move-only: Move *Disk functions to blockstorage
fa121b628d blockstorage: [refactor] Use chainman reference where possible
fadcd3f78e doc: Remove irrelevant link to GitHub Unrelated and I don't see why we should remove historical links to PRs from docs.
<details><summary>Show signature data</summary> <p>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
Good change, mostly just moves. Though I think the last commit should maybe be amended or reviewing in its own PR.
[@MarcoFalke](/github-metadata-backup-bitcoin-bitcoin/contributor/marcofalke/) I assume that the circular deps introduced here are thought to be temporary?
Built locally and ran unittest suite.
- - [x] faf843c07f refactor: Move load block thread into ChainstateManager
Would be nice to document the new `m_load_block` thread.
- - [x] fa413f07a1 move-only: Move ThreadImport to blockstorage
- - [x] fa91b2b2b3 move-only: Move AbortNode to shutdown
Seems unrelated, but maybe isn't? ACK anyway.
- - [x] fa0c7d9ad2 move-only: Move *Disk functions to blockstorage
- - [x] fa121b628d blockstorage: [refactor] Use chainman reference where possible
- - [x] fadcd3f78e doc: Remove irrelevant link to GitHub
Unrelated and I don't see why we should remove historical links to PRs from docs.
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBtv1sACgkQepNdrbLE
TwX76RAAj9pVUi6OkydJS/FoBFxIM4kC2Z/ayMhFZDrhzMhCepKU8ZAV6zJal5Bo
BFz8wkW34GCFpfSwOOPy6Wq7CNS8K5cVkgXWdZRdV7tbzJmtxSlVXVNyR3PoehD6
CJnRJ2Bcqm3S1a1DGFqGaT2dpQLdnIhpB3UQ3b+egcUE9Fw85uhdHL9N9gasKxB6
OFsvLJ7W49+2J6m94fo5Pgmt/LhdcuSBFcJOVdh70MYa80gZKVOOAdzzFLiirikf
NX7f8A3fPZCeeWLYKZ8A2Z43XxYKq6OSHtyHzr1Be5m62ZY3zCOz3dkmqzZica3E
ikGK48fMzWQBq+f0V6Rjcyt0F8aK7mzKi67aqEkuobEWfp1y57iIxNV3tFnzH0IT
IZxPScoRislqRIFj+V0/YpjvkFCYu5ODFYLukpvE6rsoHMnKAXhqXrHNxPX7K0vz
4snv1TomXlWMzG6raf0xF23p8xMGN3/2N6b+XltpGVI/oXbthZNMaNcuIQHCLwIc
glydl0koJ9QW5Erj3OX8nHOLT73DIFWlB19E228DdI0fYKmXCWhsn5lfSho7U939
6Ol+gi16tQVkmEm7eNhjjWovADS6LAb96aUKlm03Kch7L3K52Icw6585C/cY54ur
marlDQj5LN5nfyCpyU/1kTfBBsENtTcotuz1yaQHbrzGoz0YcEA=
=aS2w
-----END PGP SIGNATURE-----
</p></details>
<details><summary>Show platform data</summary> <p>
Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28
Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ -L/home/james/.local/lib CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ -I/home/james/.local/include CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default --enable-multiprocess PKG_CONFIG_PATH=/home/james/.local/lib/pkgconfig CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i
Compiler version: clang version 7.0.1-8 (tags/RELEASE_701/final)
</p></details>
Code review ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Concept ACK. Looking forward to a follow up to address the new circular dependencies, TODO comments & review comments.
@MarcoFalke I assume that the circular deps introduced here are thought to be temporary?
Yes, one of them is being solved in #21726
@MarcoFalke I notice that LoadMempool() is now done from blockstorage.cpp - somehow seems an inappropriate place now for this to be called from - it made more sense as part of an import thread in init.cpp.