refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

ryanofsky · 2023-08-04T21:14:06Z

This change makes IsInitialBlockDownload and NotifyHeaderTip functions no longer tied to individual Chainstate objects. It makes them work with the ChainstateManager object instead so code is simpler and it is no longer possible to call them incorrectly with an inactive Chainstate.

This change also makes m_cached_finished_ibd caching easier to reason about, because now there is only one cached value instead of two (for background and snapshot chainstates) so the cached IBD state now no longer gets reset when a snapshot is loaded.

There should be no change in behavior because these functions were always called on the active ChainState objects.

These changes were discussed previously #27746 (comment) and #27746 (comment) as possible followups for that PR.

DrahtBot · 2023-08-04T21:14:09Z

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	MarcoFalke, naumenkogs, dergoegge
Stale ACK	TheCharlatan

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:

#28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
#27826 (validation: log which peer sent us a header by Sjors)
#27596 (assumeutxo (2) by jamesob)
#20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)

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.

dergoegge · 2023-08-07T10:06:43Z

Concept ACK

maflcko

lgtm. Left style nits

src/validation.cpp

ryanofsky

Updated 3d70aa3 -> bda557e (pr/assumeibd.2 -> pr/assumeibd.3, compare) with suggested changes

src/validation.cpp

TheCharlatan

Why not make NotifyHeaderTip a private ChainstateManager member? The PR title seems to already hint as much.

ryanofsky · 2023-08-10T13:32:42Z

re: #28218 (review)

Why not make NotifyHeaderTip a private ChainstateManager member?

Because it doesn't access private state. I think the point of classes in c++ is to encapsulate private state. If there is a group of functions operating on public state, they should be standalone functions, so they can be split up into files and organized by functionality. This way we can avoid being permanently stuck with a sprawling mess of code like validation.cpp. The urge to make functions class members instead of standalone functions when they only use public interfaces and do not manage private state leads to bad outcomes like CWallet in wallet.cpp.

TheCharlatan

ACK bda557e

dergoegge

Code review ACK bda557e

Nice interface cleanup!

dergoegge · 2023-08-11T13:38:15Z

src/validation.cpp

@@ -2641,7 +2644,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew)
    }

    bilingual_str warning_messages;
-    if (!this->IsInitialBlockDownload()) {
+    if (!m_chainman.IsInitialBlockDownload()) {


(somewhat unrelated to this pull)

Are there any plans to remove the chainman reference from Chainstate? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of Chainstate and ChainstateManager are not well defined.

(It's probably also more or less just an artifact of the de-globalization work)

re: #28218 (comment)

Are there any plans to remove the chainman reference from Chainstate? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of Chainstate and ChainstateManager are not well defined.

I agree, and if I were cleaning this up would get rid of this back reference and make a lot of ChainState members functions into standalone functions that take explicit ChainState and ChainStateManager arguments. But I think this would mainly be an aesthetic improvement and not have a lot of consequences, because in practice after #27746 and this PR the responsibilities of Chainstate and ChainstateManager are pretty clear if you just look at the actual state the classes contain and ignore the member functions. Detaching member functions and passing references explicitly instead of explicitly are good things, but less consequential than deciding how to represent and update state, which is what this PR and #27746 are focused on.

naumenkogs · 2023-08-18T11:45:40Z

ACK bda557e

fanquake · 2023-08-18T12:57:54Z

This will need to be rebased after #27460:

  CXX      rpc/libbitcoin_node_a-signmessage.o
  CXX      rpc/libbitcoin_node_a-txoutproof.o
rpc/mempool.cpp:757:28: error: no member named 'IsInitialBlockDownload' in 'Chainstate'
            if (chainstate.IsInitialBlockDownload()) {
                ~~~~~~~~~~ ^
1 error generated.
gmake[2]: *** [Makefile:10829: rpc/libbitcoin_node_a-mempool.o] Error 1

maflcko

ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈
yZn/RDpC1vSfiuda6JpUwUzxyXXsJQLislGF0AVNnNF1NjceFmcwfzF5NfWYhjExtf0o0YmRSA9OhqhPHvt0Ag==

src/test/fuzz/process_message.cpp

src/test/fuzz/process_messages.cpp

src/test/net_tests.cpp

…ChainstateManager This change makes IsInitialBlockDownload and NotifyHeaderTip functions no longer tied to individual Chainstate objects. It makes them work with the ChainstateManager object instead so code is simpler and it is no longer possible to call them incorrectly with an inactive Chainstate. This change also makes m_cached_finished_ibd caching easier to reason about, because now there is only one cached value instead of two (for background and snapshot chainstates) so the cached IBD state now no longer gets reset when a snapshot is loaded. There should be no change in behavior because these functions were always called on the active ChainState objects. These changes were discussed previously bitcoin#27746 (comment) and bitcoin#27746 (comment) as possible followups for that PR.

ryanofsky

Rebased bda557e -> 94a98fb (pr/assumeibd.3 -> pr/assumeibd.4, compare) applying suggestions and fixing silent conflict with #27460

src/test/fuzz/process_message.cpp

src/test/fuzz/process_messages.cpp

src/test/net_tests.cpp

maflcko · 2023-08-21T09:19:38Z

re-ACK 94a98fb 🐺

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 94a98fbd1d14a4ea10629b917771d50f3191e944 🐺
CT8xcHF/XxSnBppkGSnBSZJKvavuTVcWp510ritBnxEmqwOXZKjLlzuMpi7eowa7i7ESB0Wd917CaS5A+fDpCg==

naumenkogs · 2023-08-21T09:31:56Z

ACK 94a98fb

dergoegge

reACK 94a98fb

…aderTip not require a Chainstate

ryanofsky force-pushed the pr/assumeibd branch from 509a967 to 3d70aa3 Compare August 4, 2023 21:18

This was referenced Aug 5, 2023

validation: log which peer sent us a header #27826

Open

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

During IBD, prune as much as possible until we get close to where we will eventually keep blocks #20827

Merged

maflcko approved these changes Aug 7, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot changed the title ~~assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager~~ refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager Aug 7, 2023

DrahtBot added the Refactoring label Aug 7, 2023

ryanofsky force-pushed the pr/assumeibd branch from 3d70aa3 to bda557e Compare August 8, 2023 20:16

ryanofsky commented Aug 8, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 9, 2023

assumeutxo (2) #27596

Merged

TheCharlatan reviewed Aug 10, 2023

View reviewed changes

ryanofsky changed the title ~~refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager~~ refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate Aug 10, 2023

TheCharlatan approved these changes Aug 10, 2023

View reviewed changes

fanquake requested a review from jamesob August 10, 2023 13:58

dergoegge approved these changes Aug 11, 2023

View reviewed changes

DrahtBot mentioned this pull request Aug 15, 2023

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Draft

maflcko approved these changes Aug 18, 2023

View reviewed changes

src/test/fuzz/process_message.cpp Outdated Show resolved Hide resolved

src/test/fuzz/process_messages.cpp Outdated Show resolved Hide resolved

src/test/net_tests.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Aug 18, 2023

ryanofsky force-pushed the pr/assumeibd branch from bda557e to 00a2e43 Compare August 18, 2023 16:50

ryanofsky force-pushed the pr/assumeibd branch from 00a2e43 to 94a98fb Compare August 18, 2023 16:52

ryanofsky commented Aug 18, 2023

View reviewed changes

src/test/fuzz/process_message.cpp Outdated Show resolved Hide resolved

src/test/fuzz/process_messages.cpp Outdated Show resolved Hide resolved

src/test/net_tests.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from dergoegge and naumenkogs August 21, 2023 09:19

DrahtBot requested a review from TheCharlatan August 21, 2023 09:19

DrahtBot removed the CI failed label Aug 21, 2023

DrahtBot removed the request for review from naumenkogs August 21, 2023 09:32

dergoegge approved these changes Aug 21, 2023

View reviewed changes

fanquake merged commit 723f1c6 into bitcoin:master Aug 21, 2023
15 checks passed

ryanofsky mentioned this pull request Aug 24, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023

Merge bitcoin#28218: refactor: Make IsInitialBlockDownload & NotifyHe…

5c76e06

…aderTip not require a Chainstate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

ryanofsky commented Aug 4, 2023 •

edited by fanquake

DrahtBot commented Aug 4, 2023 •

edited

dergoegge commented Aug 7, 2023

maflcko left a comment

ryanofsky left a comment

TheCharlatan left a comment

ryanofsky commented Aug 10, 2023

TheCharlatan left a comment

dergoegge left a comment

dergoegge Aug 11, 2023

ryanofsky Aug 14, 2023 •

edited

naumenkogs commented Aug 18, 2023

fanquake commented Aug 18, 2023

maflcko left a comment

ryanofsky left a comment •

edited

maflcko commented Aug 21, 2023

naumenkogs commented Aug 21, 2023

dergoegge left a comment

refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

Conversation

ryanofsky commented Aug 4, 2023 • edited by fanquake

DrahtBot commented Aug 4, 2023 • edited

Reviews

Conflicts

dergoegge commented Aug 7, 2023

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 10, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

dergoegge Aug 11, 2023

Choose a reason for hiding this comment

ryanofsky Aug 14, 2023 • edited

Choose a reason for hiding this comment

naumenkogs commented Aug 18, 2023

fanquake commented Aug 18, 2023

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

maflcko commented Aug 21, 2023

naumenkogs commented Aug 21, 2023

dergoegge left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 4, 2023 •

edited by fanquake

DrahtBot commented Aug 4, 2023 •

edited

ryanofsky Aug 14, 2023 •

edited

ryanofsky left a comment •

edited