Move pblocktree global to BlockManager #22371

MarcoFalke commented at 7:02 PM on June 29, 2021: member

The block tree db is used within BlockManager to write and read the block index, so make the db global a member variable of BlockManager.

MarcoFalke added the label Refactoring on Jun 29, 2021

MarcoFalke added the label Block storage on Jun 29, 2021

MarcoFalke force-pushed on Jun 29, 2021

jamesob commented at 7:31 PM on June 29, 2021: member

Concept ACK

practicalswift commented at 10:19 PM on June 29, 2021: contributor

Concept ACK

Nice to see yet another naked new get dressed :)

theStack commented at 9:29 AM on June 30, 2021: member

Concept ACK

DrahtBot commented at 11:45 AM on June 30, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22212 by amadeuszpawlik
#19463 by luke-jr

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.

in src/init.cpp:266 in faa0e128da outdated

 262 | @@ -263,7 +263,7 @@ void Shutdown(NodeContext& node)
 263 |                  chainstate->ResetCoinsViews();
 264 |              }
 265 |          }
 266 | -        pblocktree.reset();
 267 | +        node.chainman->m_blockman.m_block_tree_db.reset();

dongcarl commented at 5:39 PM on June 30, 2021:

Is there anything between this line and node.chainman.reset() that requires pblocktree to be gone? If not, perhaps we could simply omit this line.

MarcoFalke commented at 7:52 AM on July 1, 2021:

Good point, done

in src/test/util/setup_common.cpp:170 in faa0e128da outdated

 166 | @@ -168,8 +167,8 @@ ChainTestingSetup::~ChainTestingSetup()
 167 |      m_node.mempool.reset();
 168 |      m_node.scheduler.reset();
 169 |      m_node.chainman->Reset();
 170 | +    m_node.chainman->m_blockman.m_block_tree_db.reset();

dongcarl commented at 5:40 PM on June 30, 2021:

Same question here: since chainman owns the blockman which owns the pblocktree, perhaps the m_node.chainman.reset() is enough?

MarcoFalke commented at 7:52 AM on July 1, 2021:

Good point, done

dongcarl commented at 5:43 PM on June 30, 2021: member

Super Duper Mega Concept ACK

The less global mutable variables with static lifetimes we have, the better!

MarcoFalke force-pushed on Jul 1, 2021

DrahtBot added the label Needs rebase on Jul 1, 2021

MarcoFalke force-pushed on Jul 1, 2021

DrahtBot removed the label Needs rebase on Jul 1, 2021

theStack approved

theStack commented at 9:47 PM on July 5, 2021: member

Code-review ACK fadc339f88aa76a5e461bc1cb1b5f564be9665c8 📔

DrahtBot added the label Needs rebase on Jul 15, 2021

Move LoadBlockIndexDB to BlockManager fa27f03b49

Move pblocktree global to BlockManager faa54e3757

MarcoFalke force-pushed on Jul 15, 2021

MarcoFalke removed the label Block storage on Jul 15, 2021

MarcoFalke removed the label Needs rebase on Jul 15, 2021

MarcoFalke removed the label Refactoring on Jul 15, 2021

DrahtBot added the label UTXO Db and Indexes on Jul 15, 2021

DrahtBot added the label Validation on Jul 15, 2021

MarcoFalke removed the label UTXO Db and Indexes on Jul 15, 2021

MarcoFalke removed the label Validation on Jul 15, 2021

MarcoFalke added the label Block storage on Jul 15, 2021

MarcoFalke added the label UTXO Db and Indexes on Jul 15, 2021

MarcoFalke added the label Validation on Jul 15, 2021

MarcoFalke removed the label UTXO Db and Indexes on Jul 15, 2021

MarcoFalke removed the label Validation on Jul 15, 2021

MarcoFalke added the label Needs rebase on Jul 15, 2021

MarcoFalke added the label Refactoring on Jul 15, 2021

in src/node/blockstorage.cpp:521 in faa54e3757

 517 | @@ -518,7 +518,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
 518 |                  }
 519 |                  nFile++;
 520 |              }
 521 | -            pblocktree->WriteReindexing(false);
 522 | +            WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false));

jamesob commented at 1:33 PM on July 15, 2021:

So the new lock annotation caught this as unguarded eh?

MarcoFalke commented at 2:40 PM on July 15, 2021:

Jup

jamesob approved

jamesob commented at 1:47 PM on July 15, 2021: member

ACK faa54e375782b21cbc2761c763128131c569e903 (jamesob/ackr/22371.1.MarcoFalke.move_pblocktree_global_t)

One less global in validation. :+1:

Built locally, ran tests.

<details><summary>Show signature data</summary> <p>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK faa54e375782b21cbc2761c763128131c569e903 ([`jamesob/ackr/22371.1.MarcoFalke.move_pblocktree_global_t`](https://github.com/jamesob/bitcoin/tree/ackr/22371.1.MarcoFalke.move_pblocktree_global_t))

One less global in validation. :+1:

Built locally, ran tests.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDwO3AACgkQepNdrbLE
TwWNEA/9GuE3UUeopz0KxIQCZvWv5QUaJNqUzWZZGrJHODKirzJweEa50/Ob5Kv1
JqJw4O2vM35aC5dn3TPrg4xtVX+xOjnxnYHevi8JgVyTY1JsR3ePkypx8B3TROrb
zrFHPxNl4c7KIio150bbz+NibLeMepptP6k7LIm+mv1cOjhpnoBWwcACgl1UQeqv
WzoMIHw/3AR6TTnM7n9x9D4nRWIQv8lcUYzcMOZmJCn7d46jKP9FyqOqh7ZBucrf
Rj8aQkoLEu8oJvWTuSYlifUarQ59hL19l+p9Al8uKVSnfXXekVK8iqZOLRu7+aTJ
QF7F2jsexy4TM0Adh113BNKz+viI0trizIWNgPB4lkVJbkNQt+ZohRSdcutpO1nd
dJ8n2RYXHkMI+e07ZBNvO6KIgk9QGnGNAXxj1uvVmkTEmOgX+j59K/pyEEg64+4T
a+Am6D3OJD/12gIHSlPUA3XpDWf5o7v8RBC0r0QDmdnAb7DisvHQZtA+qRJaKQGz
AJ3U+RIplTRjG3ZX+5ZJKHCU7awO4kerZh2njvLWgDtHJ9A/n3/ZSxJ2GaH+jRjN
XH9Arpr64ZMXzz0LpIRaKCBXg23Qi2698/0aRZwCQqq9XhCvUKdlbnDyCDg4Vwpr
lnFCwLSFn7nNWhprkPW23SxJQLOQC5ti3pbABUO7OcEd240gsmU=
=HRRw
-----END PGP SIGNATURE-----

</p></details>

<details><summary>Show platform data</summary> <p>

Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin2/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin2/db4/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 CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/local/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 -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162

</p></details>

DrahtBot removed the label Needs rebase on Jul 15, 2021

theStack approved

theStack commented at 11:39 PM on July 15, 2021: member

re-ACK faa54e375782b21cbc2761c763128131c569e903 🥧

ryanofsky approved

ryanofsky commented at 3:15 PM on July 20, 2021: member

Code review ACK faa54e375782b21cbc2761c763128131c569e903. I was thinking this looked like a change Carl would like, so no surprised he Mega-acked

MarcoFalke merged this on Jul 20, 2021

MarcoFalke closed this on Jul 20, 2021

MarcoFalke deleted the branch on Jul 20, 2021

sidhujag referenced this in commit 858fa74531 on Jul 23, 2021

DrahtBot locked this on Aug 16, 2022