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:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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=.`
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.
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.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
4
5Good change, mostly just moves. Though I think the last commit should maybe be amended or reviewing in its own PR.
6[@MarcoFalke](/bitcoin-bitcoin/contributor/marcofalke/) I assume that the circular deps introduced here are thought to be temporary?
7
8Built locally and ran unittest suite.
9
10
11- - [x] faf843c07f refactor: Move load block thread into ChainstateManager
12Would be nice to document the new `m_load_block` thread.
13
14- - [x] fa413f07a1 move-only: Move ThreadImport to blockstorage
15- - [x] fa91b2b2b3 move-only: Move AbortNode to shutdown
16Seems unrelated, but maybe isn't? ACK anyway.
17
18- - [x] fa0c7d9ad2 move-only: Move *Disk functions to blockstorage
19- - [x] fa121b628d blockstorage: [refactor] Use chainman reference where possible
20- - [x] fadcd3f78e doc: Remove irrelevant link to GitHub
21Unrelated and I don't see why we should remove historical links to PRs from docs.
22
23-----BEGIN PGP SIGNATURE-----
24
25iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBtv1sACgkQepNdrbLE
26TwX76RAAj9pVUi6OkydJS/FoBFxIM4kC2Z/ayMhFZDrhzMhCepKU8ZAV6zJal5Bo
27BFz8wkW34GCFpfSwOOPy6Wq7CNS8K5cVkgXWdZRdV7tbzJmtxSlVXVNyR3PoehD6
28CJnRJ2Bcqm3S1a1DGFqGaT2dpQLdnIhpB3UQ3b+egcUE9Fw85uhdHL9N9gasKxB6
29OFsvLJ7W49+2J6m94fo5Pgmt/LhdcuSBFcJOVdh70MYa80gZKVOOAdzzFLiirikf
30NX7f8A3fPZCeeWLYKZ8A2Z43XxYKq6OSHtyHzr1Be5m62ZY3zCOz3dkmqzZica3E
31ikGK48fMzWQBq+f0V6Rjcyt0F8aK7mzKi67aqEkuobEWfp1y57iIxNV3tFnzH0IT
32IZxPScoRislqRIFj+V0/YpjvkFCYu5ODFYLukpvE6rsoHMnKAXhqXrHNxPX7K0vz
334snv1TomXlWMzG6raf0xF23p8xMGN3/2N6b+XltpGVI/oXbthZNMaNcuIQHCLwIc
34glydl0koJ9QW5Erj3OX8nHOLT73DIFWlB19E228DdI0fYKmXCWhsn5lfSho7U939
356Ol+gi16tQVkmEm7eNhjjWovADS6LAb96aUKlm03Kch7L3K52Icw6585C/cY54ur
36marlDQj5LN5nfyCpyU/1kTfBBsENtTcotuz1yaQHbrzGoz0YcEA=
37=aS2w
38-----END PGP SIGNATURE-----
0Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28
1
2Configured 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
3
4Compiled 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
5
6Compiler version: clang version 7.0.1-8 (tags/RELEASE_701/final)
@MarcoFalke I assume that the circular deps introduced here are thought to be temporary?
Yes, one of them is being solved in #21726
