validation, refactor: remove CChainState::GetMempool()

jonatack commented at 11:56 am on December 8, 2021: member

Simplify CChainState validation code by removing an unnecessary GetMempool() getter.

Reason: A trivial getter or setter adds no semantic value; unless I am missing something, the data item (CChainState::m_mempool) could just as well be public.

Relevant C++ Core Guidelines (among others) https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-public https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-protected

This change allows simplifying the tx_pool fuzzer chainstate code in the second commit.

in src/test/fuzz/tx_pool.cpp:146 in c1038a6d90 outdated

142@@ -143,7 +143,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
143     CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
144     MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
145 
146-    chainstate.SetMempool(&tx_pool);
147+    chainstate.SetMempool(tx_pool);

lsilva01 commented at 12:33 pm on December 8, 2021:

m_mempool can be accessed directly.

0    chainstate.m_mempool = &tx_pool;

jonatack commented at 1:11 pm on December 8, 2021:

thanks, done

in src/test/fuzz/tx_pool.cpp:32 in c1038a6d90 outdated

31@@ -32,9 +32,9 @@ struct MockedTxPool : public CTxMemPool {
32 class DummyChainState final : public CChainState

lsilva01 commented at 12:33 pm on December 8, 2021:

Since m_mempool is now public, there is no longer any need for the class DummyChainState.

The line 126 can also be changed.

0- auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
1+ auto& chainstate{node.chainman->ActiveChainstate()};

jonatack commented at 1:10 pm on December 8, 2021:

nice! done

lsilva01 approved

lsilva01 commented at 12:38 pm on December 8, 2021: contributor

tACK c1038a6 with some nits

validation, refactor: remove unnecessary CChainState::GetMempool() getter

Simplify CChainState validation code by removing an unnecessary trivial getter
and moving the CChainState::m_mempool data member from protected to public,
placed in the same order as the CChainState constructor.

Relevant C++ Core Guideline:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters

Also pick up a review suggestion from PR 23465.

6d1d87e7e6

test, refactor: simplify tx_pool fuzzer chainstate code b86b91a0d3

jonatack force-pushed on Dec 8, 2021

jonatack commented at 1:12 pm on December 8, 2021: member

Thank you @lsilva01, I’ve added a commit authored by you for the fuzz code simplifications.

DrahtBot added the label Validation on Dec 8, 2021

lsilva01 approved

lsilva01 commented at 1:50 pm on December 8, 2021: contributor

reACK b86b91a

shaavan approved

shaavan commented at 2:38 pm on December 8, 2021: contributor

crACK b86b91a0d30f8bb487366b2d2f8aba9b5e8741f1

jnewbery commented at 1:15 pm on December 9, 2021: member

I’m ~0 on this. I think a crucial difference between what is being proposed here and the advice in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters is that here there was only a getter and no setter. Prior to this PR, only CChainState or a derived class could update m_mempool. Now, any client code can modify it. That’s a change in the class’s contract with its clients.

jonatack commented at 2:32 pm on December 9, 2021: member

Hm. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-public might be relevant. If we want it to be encapsulated, then it should be private. Public and protected are the same category (in that guideline).

MarcoFalke commented at 2:42 pm on December 9, 2021: member

Hm. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-public might be relevant. If we want it to be encapsulated, then it should be private. Public and protected are the same category.

A reference is a non-nullable “const” pointer (For pointers there are two dimensions of const), so I don’t think comparing m_mempool with m_blockman (or other references) is fair.

jonatack commented at 3:05 pm on December 9, 2021: member

My thought isn’t about comparison with the other data members, but rather that m_mempool would preferably be either public (with less and simpler code) as done here, or private with a setter if it is intended to be encapsulated.

MarcoFalke commented at 3:45 pm on December 9, 2021: member

My reply to that would be: #23465 (review)

jonatack commented at 5:13 pm on December 9, 2021: member

After reading all the early review in #23465 I see that most of these options were previously proposed by @lsilva01; sorry about that.

If m_mempool should be encapsulated, I agree with @jnewbery and with @promag in #23465 (review).

Agree with avoiding test-only helpers. In this case there may be good counter-arguments:

making it protected instead of private appears to be only for the fuzz test as well (and isn’t explicit as to why)
it is less safe and more complex to maintain than private
it seems simpler and less error-prone to grep the codebase for a setter than to hunt for workarounds (and m_mempool is used frequently, and there is also a BlockAssembler::m_mempool private data member) or than by manually making it private and then compiling to see what fails

However, since I now see that @lsilva01 did (almost) propose for it to be private in https://github.com/bitcoin/bitcoin/commit/358f94a28176f766634d9701f1688abfcf761c28, I’m not going to update this pull to do it, but may support such a change in future review if/where makes sense.

jonatack closed this on Dec 9, 2021

DrahtBot locked this on Dec 9, 2022

validation, refactor: remove CChainState::GetMempool() #23704