validation: add Config to ChainstateManager
ClosedPublic
Actions

Authored by PiRK on Mar 6 2023, 15:31.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCab0c72d2bf5d: validation: add Config to ChainstateManager

Summary

This is inspired by https://github.com/bitcoin/bitcoin/pull/24595/commits/69675ea4e73dcf5e9dd0f94802bd3463e4262081

I need ChainstateManager.GetConsensus as a dependency of core#25168

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Mar 6 2023, 15:31

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 6 2023, 15:31

PiRK requested review of this revision.Mar 6 2023, 15:31

teamcity edited the summary of this revision. (Show Details)Mar 6 2023, 15:31

remove also CChainParams forward declaration, as we include the header

no need to review this until I figure out how to do the follow-ups.

The next commit fails on checkpoints_tests.cpp, because this test uses a different config than the global one to pass to ChainstateManager member functions as parameters.

Harbormaster completed remote builds in B22274: Diff 38319.Mar 6 2023, 15:58

Harbormaster completed remote builds in B22275: Diff 38320.Mar 6 2023, 16:04

PiRK abandoned this revision.Apr 25 2023, 06:15

rebase from hell

i still don't know how to backport the rest of core#24595, but this commit is good enough on its own and unblocks a dependency in the assumeutxo stack

PiRK edited the summary of this revision. (Show Details)Oct 3 2023, 14:10

Harbormaster completed remote builds in B25247: Diff 42511.Oct 3 2023, 14:27

Do you really need this ? Chainstate already has a public CChainParams member, can't this be used (it can be accessed from CChainstateManager.active_chainstate) instead ?

This revision now requires changes to proceed.Oct 4 2023, 08:46

In D13244#327646, @Fabien wrote:

Do you really need this ? Chainstate already has a public CChainParams member, can't this be used (it can be accessed from CChainstateManager.active_chainstate) instead ?

I'm not actually sure. https://github.com/bitcoin/bitcoin/pull/25168 make heavy use of ChainstateManager::GetParams and ChainstateManager::GetConsensus.
I verified that I can succesfully replace all of these calls by ChainstateManager::ActiveChainstate().m_params(.GetConsensus), but it is a bit more verbose, and it makes our codebase diverge from Core's on what seems to me like a point of detail.

PiRK requested review of this revision.Oct 4 2023, 11:39

Fabien accepted this revision.Oct 4 2023, 11:46

This revision is now accepted and ready to land.Oct 4 2023, 11:46

Closed by commit rABCab0c72d2bf5d: validation: add Config to ChainstateManager (authored by Anthony Towns <aj@erisian.com.au>, committed by PiRK). · Explain WhyOct 4 2023, 15:39

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCab0c72d2bf5d: validation: add Config to ChainstateManager.

PiRK mentioned this in D16259: validation: deduplicate forward declaration of Consensus::Params.May 31 2024, 15:45

PiRK mentioned this in rABC8e839c67d4b9: validation: deduplicate forward declaration of Consensus::Params.Mon, Jun 3, 13:31