kernel: make m_tip_block std::optional

Sjors commented at 7:05 pm on November 19, 2024: member

Suggested in #31297 (review)

kernel: make m_tip_block std::optional b283bc8dae

DrahtBot commented at 7:05 pm on November 19, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31325.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	tdb3, l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31297 (mining: add early return to waitTipChanged() by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Validation on Nov 19, 2024

tdb3 commented at 2:26 am on November 20, 2024: contributor

Concept ACK

Using optional seems much nicer than using a magic value. Planning to circle back to take another look.

in src/init.cpp:1798 in b283bc8dae

1794@@ -1795,7 +1795,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1795     if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip() == nullptr)) {
1796         WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
1797         kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
1798-            return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node);
1799+            return kernel_notifications.m_tip_block.has_value() || ShutdownRequested(node);

l0rinc commented at 4:49 pm on November 20, 2024:

I’ve noticed that we’re checking whether the optional has a value in different ways here and in interfaces.cpp. If you change the PR again, please consider unifying:

0            return kernel_notifications.m_tip_block || ShutdownRequested(node);

in src/node/interfaces.cpp:976 in b283bc8dae

972@@ -973,7 +973,7 @@ class MinerImpl : public Mining
973         {
974             WAIT_LOCK(notifications().m_tip_block_mutex, lock);
975             notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
976-                return (notifications().m_tip_block != current_tip && notifications().m_tip_block != uint256::ZERO) || chainman().m_interrupt;
977+                return (notifications().m_tip_block && notifications().m_tip_block.value() != current_tip) || chainman().m_interrupt;

l0rinc commented at 4:56 pm on November 20, 2024:

Since C++17 we can safely compare an optional<T> with another T:

0template< class T, class U >
1constexpr bool operator!=(const optional<T>& opt, const U& value);
2(23) (since C++17)

If you modify again, consider simplifying to:

0                return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt;

TheCharlatan commented at 9:53 pm on November 20, 2024:

Can’t you skip the first conditional too then?

ryanofsky commented at 1:46 am on November 21, 2024:

re: #31325 (review)

Can’t you skip the first conditional too then?

We do need the first condition because we want this to wait for m_tip_block to be set and for the value to be different than the current_tip value. Without the first condition this wouldn’t wait at all for m_tip_block to be set.

in src/test/validation_chainstate_tests.cpp:75 in b283bc8dae

71@@ -72,7 +72,8 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
72     ChainstateManager& chainman = *Assert(m_node.chainman);
73     const auto get_notify_tip{[&]() {
74         LOCK(m_node.notifications->m_tip_block_mutex);
75-        return m_node.notifications->m_tip_block;
76+        BOOST_REQUIRE(m_node.notifications->m_tip_block);

l0rinc commented at 4:58 pm on November 20, 2024:

Does the BOOST_REQUIRE serve here as a better error message than what m_tip_block.value() would already throw (i.e. bad_optional_access())?

l0rinc commented at 4:59 pm on November 20, 2024: contributor

Concept ACK, please see my simplification nits.

TheCharlatan commented at 9:58 pm on November 20, 2024: contributor

lgtm, but I don’t think this should be prefixed with kernel: in both the title and the commit message. The point of having the notification interface is that the node code can extend kernel behaviour. The implementation details of that are not part of the kernel.

ryanofsky approved

ryanofsky commented at 2:19 am on November 21, 2024: contributor

Code review ACK b283bc8dae0e0b714bdf1828579c67ab94da2cc7.

Can ignore this, but I want to express a slightly dissenting opinion on this change. I don’t object to it but wouldn’t have recommended it, because checking for hash.IsNull() is so widespread in the code, and I think writing interoperable code becomes more error prone when there are multiple ways of describing an unset hash instead of standardizing on one. But on the other hand I understand using std::optional can be more self-documenting and intuitive and it can force developers to check for special values (by crashing or triggering undefined behavior) in cases where they might otherwise forget to do that. So this approach probably is a little better in the long term even if it creates more complexity in the short term.

A separate concern is I think b283bc8dae0e0b714bdf1828579c67ab94da2cc7 is using the optional::value() method inappropriately. It’s good to use the value() method in code that wants to catch and throw the std::bad_optional_access exception but b283bc8dae0e0b714bdf1828579c67ab94da2cc7 is calling it in cases where exceptions could never be thrown. IMO, unless you actually want this exception, it is better to use * and -> operators because these operators stand out to people used to reading C/C++ code as places where dereferences are happening and don’t just look like innocuous method calls. These are the same reasons to prefer the vector [] operator over the vector .at() method.

DrahtBot requested review from l0rinc on Nov 21, 2024

DrahtBot requested review from tdb3 on Nov 21, 2024

kernel: make m_tip_block std::optional #31325

Code Coverage & Benchmarks

Reviews

Conflicts