validation: Move warningcache to ChainstateManager and rename to m

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

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

Reviews

Conflicts