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

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

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28622
Build 56785: Build Diff	build-clang-tidy · build-diff · lint-circular-dependencies · build-debug · build-clang · build-without-wallet
Build 56784: arc lint + arc unit

PiRK created this revision.Apr 17 2024, 15:06

PiRK requested review of this revision.Apr 17 2024, 15:06

bytesofman added inline comments.

src/test/validation_chainstate_tests.cpp
25	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	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	Yes it is a linting thing. https://www.reddit.com/r/cpp_questions/comments/vdz8ec/comment/icne5l4/
src/test/validation_flush_tests.cpp
19	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