Move pblocktree global to BlockManager

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.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK faa54e375782b21cbc2761c763128131c569e903 ([`jamesob/ackr/22371.1.MarcoFalke.move_pblocktree_global_t`](https://github.com/jamesob/bitcoin/tree/ackr/22371.1.MarcoFalke.move_pblocktree_global_t))
 4
 5One less global in validation. :+1:
 6
 7Built locally, ran tests.
 8-----BEGIN PGP SIGNATURE-----
 9
10iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDwO3AACgkQepNdrbLE
11TwWNEA/9GuE3UUeopz0KxIQCZvWv5QUaJNqUzWZZGrJHODKirzJweEa50/Ob5Kv1
12JqJw4O2vM35aC5dn3TPrg4xtVX+xOjnxnYHevi8JgVyTY1JsR3ePkypx8B3TROrb
13zrFHPxNl4c7KIio150bbz+NibLeMepptP6k7LIm+mv1cOjhpnoBWwcACgl1UQeqv
14WzoMIHw/3AR6TTnM7n9x9D4nRWIQv8lcUYzcMOZmJCn7d46jKP9FyqOqh7ZBucrf
15Rj8aQkoLEu8oJvWTuSYlifUarQ59hL19l+p9Al8uKVSnfXXekVK8iqZOLRu7+aTJ
16QF7F2jsexy4TM0Adh113BNKz+viI0trizIWNgPB4lkVJbkNQt+ZohRSdcutpO1nd
17dJ8n2RYXHkMI+e07ZBNvO6KIgk9QGnGNAXxj1uvVmkTEmOgX+j59K/pyEEg64+4T
18a+Am6D3OJD/12gIHSlPUA3XpDWf5o7v8RBC0r0QDmdnAb7DisvHQZtA+qRJaKQGz
19AJ3U+RIplTRjG3ZX+5ZJKHCU7awO4kerZh2njvLWgDtHJ9A/n3/ZSxJ2GaH+jRjN
20XH9Arpr64ZMXzz0LpIRaKCBXg23Qi2698/0aRZwCQqq9XhCvUKdlbnDyCDg4Vwpr
21lnFCwLSFn7nNWhprkPW23SxJQLOQC5ti3pbABUO7OcEd240gsmU=
22=HRRw
23-----END PGP SIGNATURE-----

0Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28
1
2Configured 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
3
4Compiled 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
5
6Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162

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

Move pblocktree global to BlockManager #22371

Conflicts