Colonel Mustard in the Lounge with the Lead Pipe (killed mapBlockIndex outside of validation) #12802

pull TheBlueMatt wants to merge 12 commits into bitcoin:master from TheBlueMatt:2018-03-const-cblockindex changing 19 files +257 −204

TheBlueMatt commented at 7:47 PM on March 27, 2018: member

Replaces #10692 after #11041 was merged. Stops exporting mapBlockIndex in validation.h entirely, also making it const inside of validation.cpp outside of CChainState. Also makes chainActive const everywhere outside of CChainState and, together, removes all non-const CBlockIndex*s outside of validation/(mostly CChainState).

Sorry, no lyrics this time.
Move LoadChainTip into CChainState fb6a932e9b
Make CBlockIndex* arguments to validation.h functions const 4de9915fea
laanwj added the label Validation on Mar 27, 2018
laanwj added the label Refactoring on Mar 27, 2018

scripted-diff: constify CBlockIndex* outside of validation

-BEGIN VERIFY SCRIPT-
sed -i "s/\(const \)*CBlockIndex\( \)*\*\( \)*/const CBlockIndex\* /g" src/checkpoints.h src/checkpoints.cpp src/rest.cpp src/rpc/*.cpp src/wallet/*.h src/wallet/*.cpp src/init.cpp src/qt/clientmodel.cpp src/miner.cpp
-END VERIFY SCRIPT-

2ecc0b0d03

Constify CBlockIndex*s in skiplist_tests where possible 98af120a46
Remove now-useless const_cast in qt/clientmodel 3cc6f5e047
Make all CBlockIndex*es in validation.h const dbed1ea3ce
mapBlockIndex only in validation.cpp, and const outside CChainState ae8e30bbb1
Make CChain only return const CBlockIndex* in const functions d1788bc499
Add a const_cast'ed reference to chainActive in miner_tests 5cf5701991

scripted-diff: Use nonCosntChainActive in miner_tests

-BEGIN VERIFY SCRIPT-
sed -i "s/chainActive\./nonConstChainActive./" src/test/miner_tests.cpp
-END VERIFY SCRIPT-

8aadafe9ce

Make VerifyDB a CChainState member instead of a loose class 11812e2fd4
Make chainActive const outside of CChainState 4576f1de67

in src/chain.cpp:72 in 4b522c3dda outdated

  68 |  }
  69 | +CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime) {
  70 | +    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime,
  71 | +        [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTimeMax() < time; });
  72 | +    return (lower == vChain.end() ? nullptr : *lower);
  73 | +}

Empact commented at 9:46 PM on March 27, 2018:

Compiles without this definition if you fix the skiplist_tests use to be const as well.

TheBlueMatt commented at 3:37 PM on March 28, 2018:

Done.

TheBlueMatt force-pushed on Mar 28, 2018
in src/validation.cpp:3852 in fb6a932e9b outdated
```
3848 | @@ -3849,26 +3849,30 @@ bool LoadChainTip(const CChainParams& chainparams)
3849 |          // that we always have a chainActive.Tip() when we return.
3850 |          LogPrintf("%s: Connecting genesis block...\n", __func__);
3851 |          CValidationState state;
3852 | -        if (!ActivateBestChain(state, chainparams)) {
3853 | +        if (!ActivateBestChain(state, chainparams, std::shared_ptr<const CBlock>())) {
```
promag commented at 11:29 AM on March 31, 2018:

In commit "Move LoadChainTip into CChainState",

Use MakeShared()?

TheBlueMatt commented at 4:09 PM on March 31, 2018:

shared_ptr() creates a pointer-to-null, not a default-constructed CBlock.

jimpo commented at 9:19 PM on May 22, 2018:

Any particular reason to pass it explicitly?

promag commented at 8:14 PM on June 10, 2018:

Indeed, this change could be reverted (it's the default value).
in src/validation.cpp:193 in fb6a932e9b outdated
```
 189 | @@ -191,6 +190,7 @@ class CChainState {
 190 |      CBlockIndex* FindMostWorkChain();
 191 |      bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBlockIndex *pindexNew, const CDiskBlockPos& pos, const Consensus::Params& consensusParams);
 192 |  
 193 | +    void PruneBlockIndexCandidates();
```
promag commented at 11:30 AM on March 31, 2018:

In commit "Move LoadChainTip into CChainState",

Nit, revert this unrelated change?

TheBlueMatt commented at 4:09 PM on March 31, 2018:

How is this unrelated? Because of the change PruneBlockIndexCandidates (an internal helper function) is no longer called outside of CChainState, so it should be made private.

promag commented at 4:42 PM on March 31, 2018:

Oh missed the visibility change.
in src/test/miner_tests.cpp:42 in 5cf5701991 outdated
```
  38 | @@ -39,6 +39,8 @@ class HasReason {
  39 |  
  40 |  static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
  41 |  
  42 | +static CChain& nonConstChainActive = const_cast<CChain&>(chainActive);
```
promag commented at 11:32 AM on March 31, 2018:

In commit "Add a const_cast'ed reference to chainActive in miner_tests",

Why add an unused variable? Could add just when it's needed?

Answer: Because later it's used in "scripted-diff: Use nonCosntChainActive in miner_tests" commit.
promag commented at 11:38 AM on March 31, 2018: member

Needs rebase. Looks good.
sipa commented at 8:27 PM on April 1, 2018: member

utACK 4576f1de6725bab583797d081421b992872fd616

Nit: typo in commit name nonCosntChainActive.
in src/validation.cpp:182 in ae8e30bbb1 outdated
```
 177 | @@ -177,6 +178,13 @@ class CChainState {
 178 |  
 179 |      void UnloadBlockIndex();
 180 |  
 181 | +    ~CChainState() {
 182 | +        BlockMap::iterator it1 = mapBlockIndex.begin();
```
jimpo commented at 9:34 PM on May 22, 2018:

nit: This can be simplified using the foreach loop in UnloadBlockIndex. And the explicit clear at the end is unnecessary.

for (BlockMap::value_type& entry : mapBlockIndex) delete entry.second;
promag commented at 8:22 PM on June 10, 2018:

Commit "mapBlockIndex only in validation.cpp, and const outside CChainState"

And the explicit clear at the end is unnecessary. @jimpo why? Otherwise it would contain dangling pointers. mapBlockIndex isn't a CChainState member.
jimpo commented at 9:42 PM on May 22, 2018: contributor

Looks good. Needs rebase.
jnewbery commented at 9:46 PM on May 22, 2018: member

PR would be more searchable with a descriptive title :)
Empact commented at 10:26 PM on May 22, 2018: member

nit: use ? in regex for 0-1 copies, rather than * (0-n copies)
sipa commented at 2:39 AM on May 28, 2018: member

Needs rebase due to #10244.
MarcoFalke added the label Needs rebase on Jun 6, 2018

in src/validation.cpp:184 in 4576f1de67

 184 | +    bool LoadChainTip(const CChainParams& chainparams);
 185 |  
 186 |      void UnloadBlockIndex();
 187 |  
 188 | +    ~CChainState() {
 189 | +        BlockMap::iterator it1 = mapBlockIndex.begin();

promag commented at 8:27 PM on June 10, 2018:

Commit "mapBlockIndex only in validation.cpp, and const outside CChainState"

Should AssertLockHeld(cs_main)?

in src/qt/clientmodel.cpp:140 in 3cc6f5e047 outdated

 136 | @@ -137,7 +137,7 @@ size_t ClientModel::getMempoolDynamicUsage() const
 137 |  
 138 |  double ClientModel::getVerificationProgress(const CBlockIndex* tipIn) const
 139 |  {
 140 | -    const CBlockIndex* tip = const_cast<const CBlockIndex* >(tipIn);
 141 | +    const CBlockIndex* tip = tipIn;

promag commented at 8:30 PM on June 10, 2018:

Commit "Remove now-useless const_cast in qt/clientmodel"

Could remove this line and rename argument to tip.

promag commented at 8:31 PM on June 10, 2018: member

Typo in commit "scripted-diff: Use nonCosntChainActive in miner_tests"
TheBlueMatt closed this on Jun 14, 2018
laanwj removed the label Needs rebase on Oct 24, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Refactoring Validation

Linked (view graph)

#10692 Make mapBlockIndex and chainActive and all CBlockIndex*es const outside of validation/CChainState #11041 Add LookupBlockIndex