-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validation: Move warningcache to ChainstateManager and rename to m_warningcache #27357
validation: Move warningcache to ChainstateManager and rename to m_warningcache #27357
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep you commits squashed according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits Otherwise it is harder to review
8bba010
to
5526849
Compare
@MarcoFalke Thanks for the remainder, squashed them and it should be good now. |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5526849
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 |
Good point, should increase the startup time significantly. I'm on it and will address it in a following PR. |
Please remove the merge commit in this branch. A merge commit will be created by the merge script. |
(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) |
683bef9
to
5526849
Compare
Somehow when I synced my forked repo with the latest changes, it merged this branch too. thx |
I just assumed that since there would be less block header processing, but what you say makes sense in that regard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The priority of this change should be pretty low anyways so I agree with that. |
FWIW, I think the rebase churn is pretty trivial (the PR's change neighbouring lines in validation.h -- this one introduces |
I think it's ok to merge this now. |
…er and rename to m_warningcache 5526849 validation: Move warningcache to ChainstateManager (dimitaracev) Pull request description: Removes `warningcache` and moves it to `ChainstateManager`. Also removes the respective `TODO` completely. ACKs for top commit: ajtowns: ACK 5526849 dimitaracev: > ACK [5526849](bitcoin@5526849) TheCharlatan: ACK 5526849 ryanofsky: Code review ACK 5526849 Tree-SHA512: 6869bd7aa4f0b59324e12eb8e3df47f2c9a3f3b0d9b7d45857426ec9e8b71c5573bdcf71db822f8c10aff7d8679a00a4bedc7a256c28f325e744e5d7267b41e9
Removes
warningcache
and moves it toChainstateManager
. Also removes the respectiveTODO
completely.