Make m_tip_block std::optional

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

Suggested in #31297 (review)

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	l0rinc
Concept ACK	tdb3, BrandonOdiwuor
Stale ACK	ryanofsky

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:

#31346 (Set notifications m_tip_block in LoadChainTip() 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 outdated

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);

Sjors commented at 10:08 am on November 21, 2024:

The reason I did it differently was that interfaces.cpp the pattern blah && blah.value() makes it clear the first value is a pointer rather than a boolean. That’s not clear here.

That said, I don’t think anyone would be confused either if I leave out .has_value()

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

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.

Sjors commented at 10:09 am on November 21, 2024:

That’s probably worth clarifying in a comment…

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

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())?

Sjors commented at 10:10 am on November 21, 2024:

I didn’t think about it that deeply, but yes.

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

Make m_tip_block an std::optional

This change avoids ambiguity when no tip is connected and it is
compared to uint256::ZERO.

2fff10a656

Sjors force-pushed on Nov 21, 2024

Sjors renamed this:
~~kernel: make m_tip_block std::optional~~
Make m_tip_block std::optional
on Nov 21, 2024

Sjors commented at 12:15 pm on November 21, 2024: member

I dropped the use of has_value() #31325 (review) as well as value() #31325 (review). Except in the test, where throwing std::bad_optional_access is probably useful.

Added a comment: https://github.com/bitcoin/bitcoin/pull/31325/files#r1851747612

Dropped the word “kernel” and added a motivation to the commit message.

l0rinc commented at 12:19 pm on November 21, 2024: contributor

ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81

DrahtBot requested review from ryanofsky on Nov 21, 2024

BrandonOdiwuor commented at 3:14 pm on November 22, 2024: contributor

Concept ACK transitioning m_tip_block to std::optional which remains unset until a block is connected

Make m_tip_block std::optional #31325

Code Coverage & Benchmarks

Reviews

Conflicts