Skip to content

test: [refactor] Use reference over ptr to chainman #33840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fanquake merged 2 commits into from
Nov 11, 2025
Merged

test: [refactor] Use reference over ptr to chainman #33840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
faf2759
test: [refactor] Use reference over ptr to chainman
Nov 10, 2025
7a4901c
test, refactor: Fix `-Warray-bounds` warning
hebasto Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions ci/test/00_setup_env_arm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ export GOAL="install"
export CI_LIMIT_STACK_SIZE=1
# -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
# This could be removed once the ABI change warning does not show up by default
#
# -Wno-error=dangling-reference helps to work around a GCC 13.1 false-positive,
# fixed in later versions.
export BITCOIN_CONFIG=" \
-DREDUCE_EXPORTS=ON \
-DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=dangling-reference -Wno-error=maybe-uninitialized' \
-DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=maybe-uninitialized' \
"
4 changes: 1 addition & 3 deletions ci/test/00_setup_env_win64.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ export PACKAGES="g++-mingw-w64-x86-64-posix nsis"
export RUN_UNIT_TESTS=false
export RUN_FUNCTIONAL_TESTS=false
export GOAL="deploy"
# -Wno-error=dangling-reference helps to work around a GCC 13.1 false-positive,
# fixed in later versions.
export BITCOIN_CONFIG="-DREDUCE_EXPORTS=ON -DBUILD_GUI_TESTS=OFF -DBUILD_KERNEL_LIB=ON -DBUILD_KERNEL_TEST=ON \
-DCMAKE_CXX_FLAGS='-Wno-error=dangling-reference -Wno-error=maybe-uninitialized' \
-DCMAKE_CXX_FLAGS='-Wno-error=maybe-uninitialized' \
"
20 changes: 10 additions & 10 deletions src/test/blockmanager_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,16 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain100Setup)
{
// Cap last block file size, and mine new block in a new block file.
const auto& chainman = Assert(m_node.chainman);
auto& blockman = chainman->m_blockman;
const CBlockIndex* old_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())};
WITH_LOCK(chainman->GetMutex(), blockman.GetBlockFileInfo(old_tip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
auto& chainman{*Assert(m_node.chainman)};
auto& blockman{chainman.m_blockman};
const CBlockIndex* old_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
WITH_LOCK(chainman.GetMutex(), blockman.GetBlockFileInfo(old_tip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));

// Prune the older block file, but don't unlink it
int file_number;
{
LOCK(chainman->GetMutex());
LOCK(chainman.GetMutex());
file_number = old_tip->GetBlockPos().nFile;
blockman.PruneOneBlockFile(file_number);
}
Expand All @@ -78,22 +78,22 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain

// Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles
// if m_have_pruned is not yet set
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
BOOST_CHECK(!blockman.OpenBlockFile(pos, true).IsNull());

// Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles
// once m_have_pruned is set
blockman.m_have_pruned = true;
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
BOOST_CHECK(blockman.OpenBlockFile(pos, true).IsNull());

// Check that calling with already pruned files doesn't cause an error
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());

// Check that the new tip file has not been removed
const CBlockIndex* new_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())};
const CBlockIndex* new_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
BOOST_CHECK_NE(old_tip, new_tip);
const int new_file_number{WITH_LOCK(chainman->GetMutex(), return new_tip->GetBlockPos().nFile)};
const int new_file_number{WITH_LOCK(chainman.GetMutex(), return new_tip->GetBlockPos().nFile)};
const FlatFilePos new_pos(new_file_number, 0);
BOOST_CHECK(!blockman.OpenBlockFile(new_pos, true).IsNull());
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/net_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,7 +1301,7 @@ class V2TransportTester
{
// Construct contents consisting of 0x00 + 12-byte message type + payload.
Copy link
Contributor

@l0rinc l0rinc Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could mention that the fixed-size type is right-padded with zeros:

Suggested change
// Construct contents consisting of 0x00 + 12-byte message type + payload.
// [0x00][12-byte message type, zero padded on right][payload]

Copy link
Member Author

@maflcko maflcko Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is clear from reading the code, so will leave as-is for now. No opinion about string_view. Will leave as-is for now, to not invalidate the two reviews.

std::vector<uint8_t> contents(1 + CMessageHeader::MESSAGE_TYPE_SIZE + payload.size());
std::copy(mtype.begin(), mtype.end(), reinterpret_cast<char*>(contents.data() + 1));
std::copy(mtype.begin(), mtype.end(), contents.begin() + 1);
std::copy(payload.begin(), payload.end(), contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
Comment on lines +1304 to 1305
Copy link
Contributor

@l0rinc l0rinc Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify that the second one doesn't actually copy where the first left off:

Suggested change
std::copy(mtype.begin(), mtype.end(), contents.begin() + 1);
std::copy(payload.begin(), payload.end(), contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
std::ranges::copy(mtype, contents.begin() + 1);
std::ranges::copy(payload, contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);

Copy link
Member Author

@maflcko maflcko Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to just change the line, so that it matches all other test cases in this file. Happy to review a follow-up to apply the clang-tidy use-ranges to the src/test directory (or all files). But I want to keep this one minimal for now.

// Send a packet with that as contents.
SendPacket(contents);
Expand Down
Loading