Make m_tip_block std::optional #31325

Sjors · 2024-11-19T19:05:22Z

DrahtBot · 2024-11-19T19:05:25Z

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.

tdb3

Concept ACK

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

l0rinc

Concept ACK, please see my simplification nits.

l0rinc · 2024-11-20T16:49:12Z

src/init.cpp

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:

Suggested change

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

return kernel_notifications.m_tip_block || ShutdownRequested(node);

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

l0rinc · 2024-11-20T16:56:12Z

src/node/interfaces.cpp

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

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

If you modify again, consider simplifying to:

Suggested change

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

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

Can't you skip the first conditional too then?

re: #31325 (comment)

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.

That's probably worth clarifying in a comment...

l0rinc · 2024-11-20T16:58:36Z

src/test/validation_chainstate_tests.cpp

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

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

sedited

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.

sedited · 2024-11-20T21:53:38Z

src/node/interfaces.cpp

Can't you skip the first conditional too then?

ryanofsky

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.

ryanofsky · 2024-11-21T01:46:04Z

src/node/interfaces.cpp

re: #31325 (comment)

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 · 2024-11-21T12:15:03Z

I dropped the use of has_value() #31325 (comment) as well as value() #31325 (comment). 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 · 2024-11-21T12:19:17Z

ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81

BrandonOdiwuor

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

ryanofsky

Code review ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81

ryanofsky · 2024-12-04T19:06:26Z

src/node/interfaces.cpp

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.

Sjors · 2024-12-06T07:33:08Z

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

l0rinc · 2024-12-06T12:40:28Z

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

src/node/interfaces.cpp

l0rinc · 2024-12-06T13:39:44Z

ACK fe6487ca6ab005faab9f1d565740f2259c04cd16

ryanofsky

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

luke-jr · 2024-12-09T16:45:30Z

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

Sjors · 2024-12-10T03:32:50Z

"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 · 2024-12-14T04:28:29Z

Rebased after #31346.

l0rinc · 2024-12-15T14:20:20Z

ACK 43f23cd29de13ea600bbe1172cf1ac0d535f66ee

Sjors · 2024-12-16T06:23:11Z

@maflcko thoughts, since you suggested it?

DrahtBot added the Validation label Nov 19, 2024

This was referenced Nov 19, 2024

mining: add early return to waitTipChanged() #31297

Closed

Stratum v2 via IPC Mining Interface tracking issue #31098

Closed

Add waitNext() to BlockTemplate interface #31283

Merged

tdb3 reviewed Nov 20, 2024

View reviewed changes

l0rinc reviewed Nov 20, 2024

View reviewed changes

sedited reviewed Nov 20, 2024

View reviewed changes

src/node/interfaces.cpp Outdated

Copy link

Contributor

sedited Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you skip the first conditional too then?

ryanofsky approved these changes Nov 21, 2024

View reviewed changes

DrahtBot requested review from l0rinc and tdb3 November 21, 2024 02:19

Sjors force-pushed the 2024/11/m_tip_block branch from b283bc8 to 2fff10a Compare November 21, 2024 12:14

Sjors changed the title ~~kernel: make m_tip_block std::optional~~ Make m_tip_block std::optional Nov 21, 2024

DrahtBot requested a review from ryanofsky November 21, 2024 12:19

BrandonOdiwuor reviewed Nov 22, 2024

View reviewed changes

DrahtBot mentioned this pull request Nov 23, 2024

Set notifications m_tip_block in LoadChainTip() #31346

Merged

ryanofsky reviewed Dec 4, 2024

View reviewed changes

Sjors force-pushed the 2024/11/m_tip_block branch from 2fff10a to 5c48d09 Compare December 6, 2024 07:33

DrahtBot requested review from BrandonOdiwuor and ryanofsky December 6, 2024 12:40

l0rinc reviewed Dec 6, 2024

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/11/m_tip_block branch from 5c48d09 to fe6487c Compare December 6, 2024 13:38

ryanofsky approved these changes Dec 6, 2024

View reviewed changes

DrahtBot added the Needs rebase label Dec 13, 2024

Sjors force-pushed the 2024/11/m_tip_block branch from fc0b5f3 to 43f23cd Compare December 14, 2024 04:28

DrahtBot removed the Needs rebase label Dec 14, 2024

	return kernel_notifications.m_tip_block.has_value() \|\| ShutdownRequested(node);
	return kernel_notifications.m_tip_block \|\| ShutdownRequested(node);

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

Make m_tip_block std::optional #31325

Make m_tip_block std::optional #31325

Uh oh!

Conversation

Sjors commented Nov 19, 2024

Uh oh!

DrahtBot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 21, 2024

Uh oh!

l0rinc commented Nov 21, 2024

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 6, 2024

Uh oh!

l0rinc commented Dec 6, 2024

Uh oh!

Uh oh!

l0rinc commented Dec 6, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 9, 2024

Uh oh!

Sjors commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 14, 2024

Uh oh!

l0rinc commented Dec 15, 2024

Uh oh!

Sjors commented Dec 16, 2024

Uh oh!

DrahtBot commented Nov 19, 2024 •

edited

Loading

Sjors Nov 21, 2024 •

edited

Loading

Sjors commented Dec 10, 2024 •

edited

Loading