refactor: Create blockstorage module by maflcko · Pull Request #21575 · bitcoin/bitcoin

maflcko · 2021-04-02T18:26:45Z

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:

Breaking down the massive files init.cpp and validation.cpp into logical units
Creating a standalone-module to reduce the mental complexity
Pave the way to fix validation related circular dependencies
Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)

DrahtBot · 2021-04-03T01:24:20Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

allow -loadblock blocks to be unsorted #20331 (allow -loadblock blocks to be unsorted by LarryRuane)
rpc: getblockfrompeer #20295 (rpc: getblockfrompeer by Sjors)
Coinstats Index #19521 (Coinstats Index by fjahr)
Add fee_est tool for debugging fee estimation code #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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

jnewbery · 2021-04-05T18:13:07Z

Concept ACK.

The include sorting is a bit off in some files.

practicalswift · 2021-04-05T18:18:56Z

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=.`

maflcko · 2021-04-05T18:29:18Z

force pushed to clang-format the include sorting

jamesob · 2021-04-05T18:38:57Z

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

promag

Code review ACK fadcd3f, 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.

maflcko · 2021-04-07T08:04:11Z

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.

jamesob

ACK fadcd3f (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.

faf843c refactor: Move load block thread into ChainstateManager
Would be nice to document the new m_load_block thread.
fa413f0 move-only: Move ThreadImport to blockstorage
fa91b2b move-only: Move AbortNode to shutdown
Seems unrelated, but maybe isn't? ACK anyway.
fa0c7d9 move-only: Move *Disk functions to blockstorage
fa121b6 blockstorage: [refactor] Use chainman reference where possible
fadcd3f doc: Remove irrelevant link to GitHub
Unrelated and I don't see why we should remove historical links to PRs from docs.

Show signature data

-----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 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-----

Show platform data

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)

jamesob · 2021-04-07T14:02:57Z

src/node/blockstorage.h

+/** Functions for disk access for blocks */
+bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
+bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);


fa0c7d9

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

ryanofsky

Code review ACK fadcd3f. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.

fanquake · 2021-04-13T14:00:18Z

Concept ACK. Looking forward to a follow up to address the new circular dependencies, TODO comments & review comments.

maflcko · 2021-04-19T06:09:31Z

@MarcoFalke I assume that the circular deps introduced here are thought to be temporary?

Yes, one of them is being solved in #21726

fa09a9e style: Add { } to multi-line if (MarcoFalke) fadafab move-only: Move functions to blockstorage (MarcoFalke) fa7e64d move-only: Move constants to blockstorage (MarcoFalke) fa247a3 refactor: Move block storage globals to blockstorage (MarcoFalke) fa81c30 refactor: Move pruning/reindex/importing globals to blockstorage (MarcoFalke) Pull request description: See #21575 ACKs for top commit: Sjors: ACK fa09a9e kiminuo: ACK fa09a9e laanwj: Code review ACK fa09a9e promag: Code review ACK fa09a9e. Since last review Tree-SHA512: 2eb6962ff44da6b77f3058fc02ec66ab742e25ae8dcc8ec62b062896571910d43ca7c4bb16fb3ccb5e5245195b8dec6384b6c8d442fa97ca28d93bdff347d677

rebroad · 2021-05-11T08:50:20Z

@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.

Summary: This is a backport of [[bitcoin/bitcoin#21575 | core#21575]] [1/6] bitcoin/bitcoin@faf843c Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11347

Summary: Can be reviewed with the git options `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space` This is a backport of [[bitcoin/bitcoin#21575 | core#21575]] [2/6] bitcoin/bitcoin@fa413f0 Depends on D11347 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11348

Summary: Can be reviewed with the git option `--color-moved=dimmed-zebra` Also fix a `if (user_message.empty())` clause which was previously negated by mistake (introduced in D8649). This caused the specific user message to be overwritten by a more generic and less useful message. This is a backport of [[bitcoin/bitcoin#21575 | core#21575]] [3/6] bitcoin/bitcoin@fa91b2b Depends on D11348 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11349

Summary: Can be reviewed with the git options `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space` This is a backport of [[bitcoin/bitcoin#21575 | core#21575]] [4/6] bitcoin/bitcoin@fa0c7d9 Depends on D11349 and D11355 Notes: - we don't have the `ReadRawBlockFromDisk` functions, which are related to segwit - some functions were in `blockdb.{h|cpp}` rather than `validation.{h|cpp}` in Bitcoin ABC. `blockdb.{h|cpp}` will most likely be unnecessary and deleted as a result of this series of backports. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11350

Summary: This is a backport of [[bitcoin/bitcoin#21575 | core#21575]] [5/6] bitcoin/bitcoin@fa121b6 Depends on D11350 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11351

maflcko mentioned this pull request Apr 2, 2021

refactor: move load block thread into ChainstateManager #21030

Closed

refactor: Move load block thread into ChainstateManager

faf843c

maflcko added Block storage Refactoring labels Apr 2, 2021

maflcko marked this pull request as ready for review April 2, 2021 18:41

maflcko force-pushed the 2104-blockstorage branch from b2c4d6f to fab062c Compare April 2, 2021 18:41

maflcko force-pushed the 2104-blockstorage branch from fab062c to 555529c Compare April 4, 2021 15:51

MarcoFalke added 2 commits April 4, 2021 18:07

move-only: Move ThreadImport to blockstorage

fa413f0

Can be reviewed with the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

move-only: Move AbortNode to shutdown

fa91b2b

Can be reviewed with the git option --color-moved=dimmed-zebra

maflcko force-pushed the 2104-blockstorage branch from 555529c to fa0248a Compare April 4, 2021 16:10

MarcoFalke added 2 commits April 5, 2021 20:26

move-only: Move *Disk functions to blockstorage

fa0c7d9

Can be reviewed with the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

blockstorage: [refactor] Use chainman reference where possible

fa121b6

Also, add missing { } for style. Can be reviewed with `--word-diff-regex=.`

maflcko force-pushed the 2104-blockstorage branch from fa0248a to fa3e883 Compare April 5, 2021 18:28

DrahtBot mentioned this pull request Apr 6, 2021

rpc: getblockfrompeer #20295

Merged

doc: Remove irrelevant link to GitHub

fadcd3f

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

DrahtBot mentioned this pull request Apr 6, 2021

Coinstats Index #19521

Merged

maflcko force-pushed the 2104-blockstorage branch from fa3e883 to fadcd3f Compare April 6, 2021 12:36

promag reviewed Apr 7, 2021

View reviewed changes

jamesob approved these changes Apr 7, 2021

View reviewed changes

bitcoin deleted a comment Apr 7, 2021

ryanofsky approved these changes Apr 10, 2021

View reviewed changes

fanquake merged commit 1f14130 into bitcoin:master Apr 13, 2021

maflcko deleted the 2104-blockstorage branch April 13, 2021 14:32

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2021

Merge bitcoin#21575: refactor: Create blockstorage module

099efbe

maflcko mentioned this pull request Apr 19, 2021

refactor: Move more stuff to blockstorage #21727

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

maflcko commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Apr 5, 2021

Uh oh!

practicalswift commented Apr 5, 2021

Uh oh!

maflcko commented Apr 5, 2021

Uh oh!

jamesob commented Apr 5, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 7, 2021

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Apr 13, 2021

Uh oh!

maflcko commented Apr 19, 2021

Uh oh!

rebroad commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maflcko commented Apr 2, 2021 •

edited

Loading

DrahtBot commented Apr 3, 2021 •

edited

Loading