Make chain state immutable outside of validation #15933

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1905-immVal changing 6 files +32 −21

MarcoFalke commented at 9:15 PM on May 1, 2019: member
This commit makes the chain state (block index map and active chain) const outside of validation. The steps are:
- Add helpers to get a const& of the chain state
- Add helpers to get a mutable reference of them to be only used in tests
- Remove mapBlockIndex from the header and mark chainActive "to be removed"
MarcoFalke commented at 9:19 PM on May 1, 2019: member

As a nice extra, this should simplify review of #15606. Also, the helpers are required by #15606.
DrahtBot commented at 9:36 PM on May 1, 2019: 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:
- #15976 (refactor: move methods under CChainState (pt. 1) by jamesob)
- #15855 ([refactor] interfaces: Add missing LockAnnotation for cs_main by MarcoFalke)
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.
DrahtBot added the label RPC/REST/ZMQ on May 1, 2019
DrahtBot added the label Tests on May 1, 2019
DrahtBot added the label Validation on May 1, 2019
DrahtBot added the label Wallet on May 1, 2019
Empact commented at 11:34 AM on May 2, 2019: member

Concept ACK - seems this would be more meaningful going forward if the const accessors to CChain returned const pointers/objects. The only thing currently disallowed to const CChain is SetTip. https://github.com/Empact/bitcoin/commits/1905-immVal
in src/validation.h:146 in fa3578a30f outdated
```
 142 | @@ -143,7 +143,6 @@ extern CCriticalSection cs_main;
 143 |  extern CBlockPolicyEstimator feeEstimator;
 144 |  extern CTxMemPool mempool;
 145 |  typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap;
 146 | -extern BlockMap& mapBlockIndex GUARDED_BY(cs_main);
```
promag commented at 2:36 PM on May 2, 2019:

Can't this be a const & instead? The same for chainActive.

Example: https://github.com/bitcoin/bitcoin/blob/24dfcf3a56f90b101bc208f48ccdb7813fa08b83/src/walletinitinterface.h#L28

MarcoFalke commented at 2:57 PM on May 2, 2019:

No, mapBlockIndex was removed from the header, so it can not be const

promag commented at 3:01 PM on May 2, 2019:

I mean instead of removing.

MarcoFalke commented at 6:26 PM on May 2, 2019:

the helpers are required by #15606 either way, so why not add them here already?
promag commented at 2:36 PM on May 2, 2019: member

Concept ACK.
jamesob commented at 6:04 PM on May 3, 2019: member

Concept ACK, but respectfully request that if we do this we call mapBlockIndex BlockIndex() instead to shorten my stay in rebase hell (https://github.com/bitcoin/bitcoin/pull/15606/commits/f2e535cdb11cbdffd3f684af89621358739cfc06).
jamesob commented at 6:06 PM on May 3, 2019: member

Would lightly prefer that we wait on this until the related bits of #15606 are in, but if you think there's compelling benefit to adding this beforehand then I'm okay with that.
practicalswift commented at 3:58 PM on May 7, 2019: contributor

Concept ACK
DrahtBot added the label Needs rebase on May 7, 2019
MarcoFalke force-pushed on May 7, 2019
MarcoFalke force-pushed on May 7, 2019
Make chain state immutable outside of validation faa48ffa2e
MarcoFalke force-pushed on May 7, 2019
DrahtBot removed the label Needs rebase on May 7, 2019
MarcoFalke commented at 10:12 PM on May 9, 2019: member

Will revisit later
MarcoFalke closed this on May 9, 2019
MarcoFalke deleted the branch on May 9, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Wallet Tests RPC/REST/ZMQ Validation

Make chain state immutable outside of validation #15933

Conflicts