HomePhabricator

CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash()

Description

CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash()

Summary:
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.

This is a partial backport of core#25349
https://github.com/bitcoin/bitcoin/pull/25349/commits/57865eb51288852c3ce99607eff76c61ae5f5365

Depends on D16071

Test Plan: ninja all check-all bitcoin-fuzzers

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D16072

Details

Provenance
Jon Atack <jon@atack.com>Authored on Jun 12 2022, 10:37
PiRKCommitted on Apr 29 2024, 18:16
PiRKPushed on Apr 29 2024, 18:16
Reviewer
Restricted Project
Differential Revision
D16072: CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash()
Parents
rABC820b59d6c5f1: DiskBlockIndex: remove unused ToString() class member
Branches
Unknown
Tags
Unknown