Page MenuHomePhabricator
Differential D16004

validation tests: Use existing {Chainstate,Block}Man
ClosedPublic
Actions

Authored by PiRK on Apr 17 2024, 15:06.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC09ee00dbb2ca: validation tests: Use existing {Chainstate,Block}Man
Summary

Use {Chain,}TestingSetup's existing {Chainstate,Block}Manager and avoid
unnecessarily creating a local one.

This also helps reduce the code diff for a later commit where we change
{Chainstate,Block}Manager's constructor signature.

This is a backport of core#25815

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Apr 17 2024, 15:06
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 17 2024, 15:06
PiRK requested review of this revision.Apr 17 2024, 15:06
teamcity edited the summary of this revision. (Show Details)Apr 17 2024, 15:06
Harbormaster completed remote builds in B28622: Diff 47239.Apr 17 2024, 15:29
bytesofman added a subscriber: bytesofman.Apr 17 2024, 19:41
bytesofman added inline comments.
src/test/validation_chainstate_tests.cpp
25 ↗(On Diff #47239)

in the core diff, this is

ChainstateManager& manager = *Assert(m_node.chainman);
vs here
ChainstateManager &manager = *Assert(m_node.chainman);

why do we have the &manager and they have ChainstateManager&? Is this just a linting thing?

src/test/validation_flush_tests.cpp
19 ↗(On Diff #47239)

also subtly different from the core diff, which uses

CChainState& chainstate{m_node.chainman->ActiveChainstate()};

Are these deltas intentional / expected?

PiRK added inline comments.Apr 17 2024, 19:48
src/test/validation_chainstate_tests.cpp
25 ↗(On Diff #47239)

Yes it is a linting thing. https://www.reddit.com/r/cpp_questions/comments/vdz8ec/comment/icne5l4/

src/test/validation_flush_tests.cpp
19 ↗(On Diff #47239)

See previous answer for the position of the &.
Regarding the CChainState vs Chainstate difference, it is because of backports out of order. We already backported the renaming of that class, which was merged in Core after this particular pull request. See D13126

PiRK added a child revision: D16006: Remove reindex special case from the progress bar label.Apr 17 2024, 19:53
bytesofman accepted this revision.Apr 17 2024, 20:32
bytesofman added inline comments.
src/test/validation_chainstate_tests.cpp
25 ↗(On Diff #47239)

lol

image.png (116×403 px, 14 KB)

This revision is now accepted and ready to land.Apr 17 2024, 20:32
Closed by commit rABC09ee00dbb2ca: validation tests: Use existing {Chainstate,Block}Man (authored by Carl Dong <contact@carldong.me>, committed by PiRK). · Explain WhyApr 18 2024, 10:02
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC09ee00dbb2ca: validation tests: Use existing {Chainstate,Block}Man.

Revision Contents
Changeset List

Diff 47264

View Options

src/test/validation_chainstate_tests.cpp

Loading...
View Options

src/test/validation_flush_tests.cpp

Loading...