validation: Move warningcache to ChainstateManager and rename to m_warningcache #27357

pull dimitaracev wants to merge 1 commits into bitcoin:master from dimitaracev:refactor-move-warningcache-to-chainstatemanager changing 2 files +3 −8

dimitaracev commented at 9:01 PM on March 28, 2023: contributor

Removes warningcache and moves it to ChainstateManager. Also removes the respective TODO completely.
DrahtBot commented at 9:01 PM on March 28, 2023: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK TheCharlatan, ajtowns, ryanofsky

Concept ACK dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #27596 (assumeutxo (2) by jamesob)
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 Validation on Mar 28, 2023
in src/validation.cpp:5595 in e2dd155bde outdated
```
5591 | @@ -5594,8 +5592,7 @@ ChainstateManager::~ChainstateManager()
5592 |  
5593 |      m_versionbitscache.Clear();
5594 |  
5595 | -    // TODO: The warning cache should probably become non-global
5596 | -    for (auto& i : warningcache) {
5597 | +    for (auto& i : m_warningcache) {
```
ajtowns commented at 9:10 PM on March 28, 2023:

This loop can be dropped entirely, no? The destructor's already running, so m_warningcache will be deleted momentarily too.

dimitaracev commented at 10:04 PM on March 28, 2023:

Makes sense, didn't want to change much since its my first PR. Removed it with 8bba010
ajtowns commented at 9:15 PM on March 28, 2023: contributor

Concept ACK
in src/validation.h:896 in 8bba010d70 outdated
```
 892 | @@ -893,6 +893,8 @@ class ChainstateManager
 893 |      //! that call.
 894 |      std::unique_ptr<Chainstate> m_snapshot_chainstate GUARDED_BY(::cs_main);
 895 |  
 896 | +    std::array<ThresholdConditionCache, VERSIONBITS_NUM_BITS> m_warningcache GUARDED_BY(::cs_main);
```
maflcko commented at 10:17 AM on March 29, 2023:

nit: Any reason to order this in-between the two chainstate pointers? Might be better to change the order logically

dimitaracev commented at 11:45 AM on March 29, 2023:

No reason, moved it more down with 5526849 .
maflcko commented at 10:17 AM on March 29, 2023: member

Please keep you commits squashed according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits Otherwise it is harder to review
validation: Move warningcache to ChainstateManager 552684976b
dimitaracev force-pushed on Mar 29, 2023
dimitaracev commented at 11:46 AM on March 29, 2023: contributor

@MarcoFalke Thanks for the remainder, squashed them and it should be good now.
dergoegge commented at 2:05 PM on March 29, 2023: member

Concept ACK
dimitaracev requested review from ajtowns on Mar 30, 2023
dimitaracev removed review request from ajtowns on Mar 30, 2023
dimitaracev requested review from maflcko on Mar 30, 2023
dimitaracev removed review request from maflcko on Mar 30, 2023
dimitaracev requested review from ajtowns on Mar 30, 2023
TheCharlatan approved
TheCharlatan commented at 12:45 PM on April 3, 2023: contributor

ACK 552684976b6df34ce563458f73812e6e494e3b0e
ajtowns commented at 6:57 AM on April 5, 2023: contributor

ACK 552684976b6df34ce563458f73812e6e494e3b0e

Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing MinBIP9WarningHeight with MinBIP9WarningStartTime (set it to say 2022-07-01 initially), and using it for WarningBitsConditionChecker::BeginTime(), and just bumping it at each release might be worthwhile.
DrahtBot removed review request from ajtowns on Apr 5, 2023
dimitaracev commented at 10:49 AM on April 5, 2023: contributor

ACK 5526849

Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing MinBIP9WarningHeight with MinBIP9WarningStartTime (set it to say 2022-07-01 initially), and using it for WarningBitsConditionChecker::BeginTime(), and just bumping it at each release might be worthwhile.

Good point, should increase the startup time significantly. I'm on it and will address it in a following PR.
maflcko commented at 7:16 AM on April 6, 2023: member

Please remove the merge commit in this branch. A merge commit will be created by the merge script.
ajtowns commented at 7:28 AM on April 6, 2023: contributor

Good point, should increase the startup time significantly. I'm on it and will address it in a following PR.

(For the record I don't think this will affect startup time significantly and that you'd need to add bench logging code to even observe the difference outside of the noise of disk reads and reconnecting on the network)
dimitaracev force-pushed on Apr 6, 2023
dimitaracev commented at 10:52 AM on April 6, 2023: contributor

Please remove the merge commit in this branch. A merge commit will be created by the merge script.

Somehow when I synced my forked repo with the latest changes, it merged this branch too. thx
dimitaracev commented at 10:59 AM on April 6, 2023: contributor

Good point, should increase the startup time significantly. I'm on it and will address it in a following PR.

(For the record I don't think this will affect startup time significantly and that you'd need to add bench logging code to even observe the difference outside of the noise of disk reads and reconnecting on the network)

I just assumed that since there would be less block header processing, but what you say makes sense in that regard.
ryanofsky approved
ryanofsky commented at 5:39 PM on June 9, 2023: contributor

Code review ACK 552684976b6df34ce563458f73812e6e494e3b0e

This change does seem ready to merge, but I notice it conflicts with #27596 so might not be worth creating more rebase churn there. Will leave alone for now and wait for more input or a better opportunity to merge
DrahtBot requested review from ryanofsky on Jun 9, 2023
DrahtBot removed review request from ryanofsky on Jun 9, 2023
dimitaracev commented at 6:06 PM on June 9, 2023: contributor

The priority of this change should be pretty low anyways so I agree with that.
ajtowns commented at 10:04 AM on June 10, 2023: contributor

This change does seem ready to merge, but I notice it conflicts with #27596 so might not be worth creating more rebase churn there. Will leave alone for now and wait for more input or a better opportunity to merge

FWIW, I think the rebase churn is pretty trivial (the PR's change neighbouring lines in validation.h -- this one introduces m_warningcache while assumeutxo moves GetSnapshotBaseHeight), #27596 needs rebase anyway (to combine the fixups at least), and it doesn't conflict with #27746 which I think is the next part of assumevalid, though is still draft.
fanquake commented at 12:19 PM on June 12, 2023: member

I think it's ok to merge this now.
fanquake merged this on Jun 12, 2023
fanquake closed this on Jun 12, 2023
sidhujag referenced this in commit 6810d67d63 on Jun 12, 2023
bitcoin locked this on Jun 11, 2024