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	tdb3, l0rinc, fjahr
Concept ACK	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

No conflicts as of last run.

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

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

in src/node/interfaces.cpp:977 in 2fff10a656 outdated

972@@ -973,7 +973,10 @@ 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+                // Although C++17 allows safe comparison of optional<T> with
978+                // another T, we need to wait for m_tip_block to be set AND

ryanofsky commented at 7:06 pm on December 4, 2024:

In commit “Make m_tip_block an std::optional” (2fff10a656edb8bec7d45433bf431fd233d0fd81)

IMO would be less confusing to drop comment about c++17 and just say “We need to wait for m_tip_block to be set AND for the value to be different than the current_tip value” which explains why there are two conditions.

ryanofsky commented at 7:08 pm on December 4, 2024: contributor

Code review ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81

Sjors force-pushed on Dec 6, 2024

Sjors commented at 7:33 am on December 6, 2024: member

Rebased (just in case) and shortened the comment: #31325 (review)

l0rinc commented at 12:40 pm on December 6, 2024: contributor

ACK 5c48d098dc300ae47f2ffeb9305f58bd6352152d The only change was to the comment which I find less technical and more descriptive 👍

DrahtBot requested review from ryanofsky on Dec 6, 2024

DrahtBot requested review from BrandonOdiwuor on Dec 6, 2024

in src/node/interfaces.cpp:977 in 5c48d098dc outdated

972@@ -973,7 +973,9 @@ 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+                // We need to wait for m_tip_block to be set AND for the value
978+                // to be different than the current_tip value.

l0rinc commented at 12:47 pm on December 6, 2024:

nit: https://languagetool.org complains here

0Did you mean 'different from'? 'Different than' is often considered colloquial style. 
1Incorrect:  Are human beings any different than animals? 
2Correct:  Are human beings any different from animals?

0                // We need to wait for m_tip_block to be set AND for the value
1                // to differ from the current_tip value.

Sjors commented at 1:38 pm on December 6, 2024:

Taken

Sjors force-pushed on Dec 6, 2024

l0rinc commented at 1:39 pm on December 6, 2024: contributor

ACK fe6487ca6ab005faab9f1d565740f2259c04cd16

ryanofsky approved

ryanofsky commented at 2:33 pm on December 6, 2024: contributor

Code review ACK fe6487ca6ab005faab9f1d565740f2259c04cd16 just taking suggesting code comment changes since last review. (Thanks for the updates!)

luke-jr commented at 4:45 pm on December 9, 2024: member

Not sure this is an improvement. Seems like a case of “two ways to be omitted” which has caused issues in the past.

Sjors commented at 3:32 am on December 10, 2024: member

“two ways to be omitted”

If this change is merged, we should discourage the use of uint256::ZERO / uint256{}. But there are other places in the codebase that still use this pattern, which I didn’t refactor here. I even found myself using it yesterday for an upcoming PR.

I pushed a commit that makes m_tip_block private, accessible through TipBlock(). I then added an Assert at the only place that sets it to ensure it’s not ZERO.

Also modified the first commit to drop a redundant “for” from the m_tip_block comment.

Sjors force-pushed on Dec 10, 2024

in src/node/kernel_notifications.cpp:56 in 0f52501c46 outdated

51@@ -52,6 +52,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
52 {
53     {
54         LOCK(m_tip_block_mutex);
55+        Assert(index.GetBlockHash() != uint256{});
56         m_tip_block = index.GetBlockHash();

l0rinc commented at 9:41 am on December 10, 2024:

I think this should either be an assert or an Assume instead.

Assume

Should be used to run non-fatal checks. In debug builds it behaves like

Assert()/assert() to notify developers and testers about non-fatal errors.

In production it doesn’t warn or log anything.

And in most places I saw the !....IsNull() pattern instead: https://github.com/bitcoin/bitcoin/blob/master/src/txdb.cpp#L97, https://github.com/bitcoin/bitcoin/blob/master/src/wallet/scriptpubkeyman.cpp#L1000, https://github.com/bitcoin/bitcoin/blob/master/src/primitives/transaction.cpp#L77, https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2490, etc have have

And it would be simpler to check after assignment.

0        m_tip_block = index.GetBlockHash();
1        assert(!m_tip_block->IsNull());

Sjors commented at 10:46 am on December 10, 2024:

I think newer code is trying to avoid assert to prevent opening production nodes open to attacks like https://bitcoincore.org/en/2024/10/08/disclose-blocktxn-crash/

Looking at developer-notes.md section on asserts, I think Assume is better. However it depends on whether a mistake here is dangerous, which hard to say.

I find IsNull() too ambiguous for use with an std::optional.

l0rinc commented at 11:05 am on December 10, 2024:

I find IsNull() too ambiguous for use with an std::optional.

My mistake, I meant to write !m_tip_block->IsNull(), i.e. it’s not on the optional

l0rinc commented at 11:08 am on December 10, 2024:

Or you only want to check that it’s either empty or if not it’s not 0? In that case this seems like the simplest indeed

Sjors commented at 3:20 am on December 13, 2024:

GetBlockHash() is not an std::optional, so I only want to make sure it’s not 0.

After this is checked, we assign the value to m_tip_block which is an std::optional, but at that point I’m not worried that m_tip_block remains unset.

in src/node/kernel_notifications.cpp:106 in 0f52501c46 outdated

 99@@ -99,6 +100,14 @@ void KernelNotifications::fatalError(const bilingual_str& message)
100                     m_exit_status, message, &m_warnings);
101 }
102 
103+std::optional<uint256> KernelNotifications::TipBlock()
104+{
105+    AssertLockHeld(m_tip_block_mutex);
106+    Assert(!m_tip_block || m_tip_block.value() != uint256{});

l0rinc commented at 9:42 am on December 10, 2024:

we’re already doing validation during assignment, this seems redundant to me

Sjors commented at 10:47 am on December 10, 2024:

It is now, but maybe we introduce assignment somewhere else and forget to check it there. This would catch such a mistake.

It’s a bit overkill, which we could drop once the whole codebase uses a single pattern.

l0rinc commented at 11:06 am on December 10, 2024:

It’s a bit overkill, which we could drop once the whole codebase uses a single pattern.

Valid argument

in src/node/kernel_notifications.h:70 in 0f52501c46 outdated

69 private:
70     const std::function<bool()>& m_shutdown_request;
71     std::atomic<int>& m_exit_status;
72     node::Warnings& m_warnings;
73+
74+    // May not be ZERO.

l0rinc commented at 9:44 am on December 10, 2024:

Same, comment seems excessive, the code already makes this clear

in src/test/validation_chainstate_tests.cpp:76 in 0f52501c46 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->TipBlock());
77+        return m_node.notifications->TipBlock().value();

l0rinc commented at 9:44 am on December 10, 2024:

nit:

0        return *m_node.notifications->TipBlock();

Sjors commented at 10:49 am on December 10, 2024:

Done

Sjors force-pushed on Dec 10, 2024

Sjors commented at 10:58 am on December 10, 2024: member

I switched from Assert to Assume.

l0rinc commented at 11:09 am on December 10, 2024: contributor

ACK fc0b5f3b77166aad9fb288b8eb9185bb2dc7496c

DrahtBot requested review from ryanofsky on Dec 10, 2024

DrahtBot added the label Needs rebase on Dec 13, 2024

Sjors force-pushed on Dec 14, 2024

Sjors commented at 4:28 am on December 14, 2024: member

Rebased after #31346.

DrahtBot removed the label Needs rebase on Dec 14, 2024

l0rinc commented at 2:20 pm on December 15, 2024: contributor

ACK 43f23cd29de13ea600bbe1172cf1ac0d535f66ee

Sjors commented at 6:23 am on December 16, 2024: member

@maflcko thoughts, since you suggested it?

in src/node/kernel_notifications.h:59 in d66f458ea2 outdated

55@@ -56,10 +56,11 @@ class KernelNotifications : public kernel::Notifications
56 
57     Mutex m_tip_block_mutex;
58     std::condition_variable m_tip_block_cv GUARDED_BY(m_tip_block_mutex);
59+

fjahr commented at 3:09 pm on December 16, 2024:

nit: Empty line not needed here?

Sjors commented at 2:33 am on December 17, 2024:

I find that it makes the next comment more readable.

Sjors commented at 2:46 am on December 17, 2024:

I have to retouch, so will drop it.

in src/node/kernel_notifications.cpp:106 in 43f23cd29d outdated

 99@@ -99,6 +100,14 @@ void KernelNotifications::fatalError(const bilingual_str& message)
100                     m_exit_status, message, &m_warnings);
101 }
102 
103+std::optional<uint256> KernelNotifications::TipBlock()
104+{
105+    AssertLockHeld(m_tip_block_mutex);
106+    Assume(!m_tip_block || m_tip_block.value() != uint256{});

fjahr commented at 6:32 pm on December 16, 2024:

nit: I tend to agree with other commenters that explicitly checking for non-zero and the comment about it might not be needed but I am not too passionate about it. Similarly I would add to that that this line here could have used uint256::ZERO like the code that was changed previously.

tdb3 commented at 2:34 am on December 17, 2024:

nit: Using uint256::ZERO seems clearer to me, but I don’t feel too strongly about it.

Sjors commented at 2:38 am on December 17, 2024:

There was another pull request where someone suggested the use of uint256{} instead of uint256::ZERO, though I forgot where. The codebase uses both. Though in this context perhaps ZERO makes the intention more clear.

Sjors commented at 2:47 am on December 17, 2024:

I droppend this check, only keeping the setter.

fjahr commented at 6:49 pm on December 16, 2024: contributor

utACK 43f23cd29de13ea600bbe1172cf1ac0d535f66ee

in src/node/kernel_notifications.cpp:55 in 43f23cd29d outdated

51@@ -52,6 +52,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
52 {
53     {
54         LOCK(m_tip_block_mutex);
55+        Assume(index.GetBlockHash() != uint256{});

tdb3 commented at 2:34 am on December 17, 2024:

nit: Same here (uint256::ZERO)

Sjors commented at 2:47 am on December 17, 2024:

Done here

tdb3 approved

tdb3 commented at 2:40 am on December 17, 2024: contributor

Code review ACK 43f23cd29de13ea600bbe1172cf1ac0d535f66ee

Left a pico nit, but nothing blocking.

Sjors force-pushed on Dec 17, 2024

Sjors commented at 2:47 am on December 17, 2024: member

I replaced the use of uint256{} with uint256::ZERO, since it makes the intention more clear. I also limited the not-ZERO check to just the setter and dropped the comment.

Also dropped the whitespace change in kernel_notifications.h in the first commit.

in src/node/kernel_notifications.h:69 in 1991286dca outdated

65 private:
66     const std::function<bool()>& m_shutdown_request;
67     std::atomic<int>& m_exit_status;
68     node::Warnings& m_warnings;
69+
70+    std::optional<uint256> m_tip_block GUARDED_BY(m_tip_block_mutex){};

tdb3 commented at 3:00 am on December 17, 2024:

Is the {} initialization here initializing to uint256::ZERO instead of nullopt? Either way, a comment or explicit use of nulltopt/uint256::ZERO could help clarify intent.

Sjors commented at 3:15 am on December 17, 2024:

I’ll drop the {} so it should be clear that it inits to nullopt. I’m fairly sure the current syntax also does that and is not the same as {uint256{}} or = uint256{}.

tdb3 commented at 3:01 am on December 17, 2024: contributor

Approach ACK

Make m_tip_block an std::optional

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

e058544d0e

Ensure m_tip_block is never ZERO

To avoid future code changes from reintroducing the ambiguity fixed
by the previous commit, mark m_tip_block private and Assume that
it's not set to uint256::ZERO.

81cea5d4ee

Sjors force-pushed on Dec 17, 2024

tdb3 approved

tdb3 commented at 3:47 am on December 17, 2024: contributor

code review re ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9

DrahtBot requested review from fjahr on Dec 17, 2024

DrahtBot requested review from l0rinc on Dec 17, 2024

l0rinc approved

l0rinc commented at 10:22 am on December 17, 2024: contributor

ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9

fjahr commented at 1:48 pm on December 19, 2024: contributor

re-ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9

ryanofsky merged this on Dec 19, 2024

ryanofsky closed this on Dec 19, 2024

Sjors deleted the branch on Dec 20, 2024

Make m_tip_block std::optional #31325

Code Coverage & Benchmarks

Reviews

Conflicts