Page MenuHomePhabricator

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

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 requested review of this revision.Apr 17 2024, 15:06
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?

src/test/validation_chainstate_tests.cpp
25 ↗(On Diff #47239)
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

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