mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-02-05 10:17:30 -05:00
57865eb512
and mark the inherited CBlockIndex#GetBlockHash public interface member
as deleted, to disallow calling it in the derived CDiskBlockIndex class.
Here is a failing test on master demonstrating the inconsistent behavior of the
current design: calling the same inherited public interface functions on the
same CDiskBlockIndex object should yield identical behavior.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
+
```
The GetBlockHash() test assertion only passes on master because the different
methods invoked by the current design happen to return the same result. If one
of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect
design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item
36). Class usage is confusing when the behavior depends on the pointer
definition instead of the object definition (static binding happening where
dynamic binding was expected). This can lead to unsuspected or hard-to-track
bugs.
Outside of critical hot spots, correctness usually comes before optimisation,
but the current design dates back to main.cpp and it may possibly have been
chosen to avoid the overhead of dynamic dispatch. This solution does the same:
the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of
inheritance or that separate the public interface from the private
implementations. One example of the latter would be a non-virtual public
interface that calls private virtual implementation methods, i.e. the Template
pattern via the Non-Virtual Interface (NVI) idiom.
67 lines
2.3 KiB
C++
67 lines
2.3 KiB
C++
// Copyright (c) 2020-2021 The Bitcoin Core developers
|
|
// Distributed under the MIT software license, see the accompanying
|
|
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
|
|
|
|
#include <chain.h>
|
|
#include <test/fuzz/FuzzedDataProvider.h>
|
|
#include <test/fuzz/fuzz.h>
|
|
#include <test/fuzz/util.h>
|
|
|
|
#include <cstdint>
|
|
#include <optional>
|
|
#include <vector>
|
|
|
|
FUZZ_TARGET(chain)
|
|
{
|
|
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
|
|
std::optional<CDiskBlockIndex> disk_block_index = ConsumeDeserializable<CDiskBlockIndex>(fuzzed_data_provider);
|
|
if (!disk_block_index) {
|
|
return;
|
|
}
|
|
|
|
const uint256 zero{};
|
|
disk_block_index->phashBlock = &zero;
|
|
{
|
|
LOCK(::cs_main);
|
|
(void)disk_block_index->ConstructBlockHash();
|
|
(void)disk_block_index->GetBlockPos();
|
|
(void)disk_block_index->GetBlockTime();
|
|
(void)disk_block_index->GetBlockTimeMax();
|
|
(void)disk_block_index->GetMedianTimePast();
|
|
(void)disk_block_index->GetUndoPos();
|
|
(void)disk_block_index->HaveTxsDownloaded();
|
|
(void)disk_block_index->IsValid();
|
|
}
|
|
|
|
const CBlockHeader block_header = disk_block_index->GetBlockHeader();
|
|
(void)CDiskBlockIndex{*disk_block_index};
|
|
(void)disk_block_index->BuildSkip();
|
|
|
|
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
|
|
const BlockStatus block_status = fuzzed_data_provider.PickValueInArray({
|
|
BlockStatus::BLOCK_VALID_UNKNOWN,
|
|
BlockStatus::BLOCK_VALID_RESERVED,
|
|
BlockStatus::BLOCK_VALID_TREE,
|
|
BlockStatus::BLOCK_VALID_TRANSACTIONS,
|
|
BlockStatus::BLOCK_VALID_CHAIN,
|
|
BlockStatus::BLOCK_VALID_SCRIPTS,
|
|
BlockStatus::BLOCK_VALID_MASK,
|
|
BlockStatus::BLOCK_HAVE_DATA,
|
|
BlockStatus::BLOCK_HAVE_UNDO,
|
|
BlockStatus::BLOCK_HAVE_MASK,
|
|
BlockStatus::BLOCK_FAILED_VALID,
|
|
BlockStatus::BLOCK_FAILED_CHILD,
|
|
BlockStatus::BLOCK_FAILED_MASK,
|
|
BlockStatus::BLOCK_OPT_WITNESS,
|
|
});
|
|
if (block_status & ~BLOCK_VALID_MASK) {
|
|
continue;
|
|
}
|
|
WITH_LOCK(::cs_main, (void)disk_block_index->RaiseValidity(block_status));
|
|
}
|
|
|
|
CBlockIndex block_index{block_header};
|
|
block_index.phashBlock = &zero;
|
|
(void)block_index.GetBlockHash();
|
|
(void)block_index.ToString();
|
|
}
|