From 14aeece462b149eaf0d28a37d55cc169df99b2cb Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 16 Jul 2022 13:22:46 +0200 Subject: [PATCH 1/4] CBlockIndex: ensure phashBlock is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller. This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed. Aborted instead of Segmentation fault --- src/chain.h | 1 + src/validation.cpp | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chain.h b/src/chain.h index ecc2ae732f..1a499b07cc 100644 --- a/src/chain.h +++ b/src/chain.h @@ -263,6 +263,7 @@ public: uint256 GetBlockHash() const { + assert(phashBlock != nullptr); return *phashBlock; } diff --git a/src/validation.cpp b/src/validation.cpp index 4c694a2c21..d53661f1f2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2263,7 +2263,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, m_blockman.m_dirty_blockindex.insert(pindex); } - assert(pindex->phashBlock); // add this block to the view's block chain view.SetBestBlock(pindex->GetBlockHash()); From 99e8ec8721a52cd08bdca31f6e926c9c1ce281fb Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 12 Jun 2022 12:38:38 +0200 Subject: [PATCH 2/4] CDiskBlockIndex: remove unused ToString() class member and mark its inherited CBlockIndex#ToString public interface member as deleted, to disallow calling it in the derived CDiskBlockIndex class. --- src/chain.h | 11 +---------- src/test/fuzz/chain.cpp | 1 - 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/chain.h b/src/chain.h index 1a499b07cc..251471befb 100644 --- a/src/chain.h +++ b/src/chain.h @@ -415,16 +415,7 @@ public: return block.GetHash(); } - - std::string ToString() const - { - std::string str = "CDiskBlockIndex("; - str += CBlockIndex::ToString(); - str += strprintf("\n hashBlock=%s, hashPrev=%s)", - GetBlockHash().ToString(), - hashPrev.ToString()); - return str; - } + std::string ToString() = delete; }; /** An in-memory indexed chain of blocks. */ diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 8c0ed32d51..1d6b5f30db 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -31,7 +31,6 @@ FUZZ_TARGET(chain) (void)disk_block_index->GetUndoPos(); (void)disk_block_index->HaveTxsDownloaded(); (void)disk_block_index->IsValid(); - (void)disk_block_index->ToString(); } const CBlockHeader block_header = disk_block_index->GetBlockHeader(); From 57865eb51288852c3ce99607eff76c61ae5f5365 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 12 Jun 2022 12:37:41 +0200 Subject: [PATCH 3/4] CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() 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. --- src/chain.h | 3 ++- src/test/fuzz/chain.cpp | 2 +- src/txdb.cpp | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/chain.h b/src/chain.h index 251471befb..6b2e4367e9 100644 --- a/src/chain.h +++ b/src/chain.h @@ -403,7 +403,7 @@ public: READWRITE(obj.nNonce); } - uint256 GetBlockHash() const + uint256 ConstructBlockHash() const { CBlockHeader block; block.nVersion = nVersion; @@ -415,6 +415,7 @@ public: return block.GetHash(); } + uint256 GetBlockHash() = delete; std::string ToString() = delete; }; diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 1d6b5f30db..01edb06138 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -23,7 +23,7 @@ FUZZ_TARGET(chain) disk_block_index->phashBlock = &zero; { LOCK(::cs_main); - (void)disk_block_index->GetBlockHash(); + (void)disk_block_index->ConstructBlockHash(); (void)disk_block_index->GetBlockPos(); (void)disk_block_index->GetBlockTime(); (void)disk_block_index->GetBlockTimeMax(); diff --git a/src/txdb.cpp b/src/txdb.cpp index c048c2d92a..bad3bb80a9 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -310,7 +310,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, CDiskBlockIndex diskindex; if (pcursor->GetValue(diskindex)) { // Construct block index object - CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash()); + CBlockIndex* pindexNew = insertBlockIndex(diskindex.ConstructBlockHash()); pindexNew->pprev = insertBlockIndex(diskindex.hashPrev); pindexNew->nHeight = diskindex.nHeight; pindexNew->nFile = diskindex.nFile; From 3a61fc56a0ad6ed58570350dcfd9ed2d10239b48 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 16 Jul 2022 15:19:03 +0200 Subject: [PATCH 4/4] refactor: move CBlockIndex#ToString() from header to implementation which allows dropping tinyformat.h from the header file. --- src/chain.cpp | 7 +++++++ src/chain.h | 9 +-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/chain.cpp b/src/chain.cpp index b8158f7b0b..0f898bafd5 100644 --- a/src/chain.cpp +++ b/src/chain.cpp @@ -4,6 +4,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include std::string CBlockFileInfo::ToString() const @@ -11,6 +12,12 @@ std::string CBlockFileInfo::ToString() const return strprintf("CBlockFileInfo(blocks=%u, size=%u, heights=%u...%u, time=%s...%s)", nBlocks, nSize, nHeightFirst, nHeightLast, FormatISO8601Date(nTimeFirst), FormatISO8601Date(nTimeLast)); } +std::string CBlockIndex::ToString() const +{ + return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)", + pprev, nHeight, hashMerkleRoot.ToString(), GetBlockHash().ToString()); +} + void CChain::SetTip(CBlockIndex *pindex) { if (pindex == nullptr) { vChain.clear(); diff --git a/src/chain.h b/src/chain.h index 6b2e4367e9..627a3dfab2 100644 --- a/src/chain.h +++ b/src/chain.h @@ -11,7 +11,6 @@ #include #include #include -#include #include #include @@ -302,13 +301,7 @@ public: return pbegin[(pend - pbegin) / 2]; } - std::string ToString() const - { - return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)", - pprev, nHeight, - hashMerkleRoot.ToString(), - GetBlockHash().ToString()); - } + std::string ToString() const; //! Check whether this block index entry is valid up to the passed validity level. bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const