refactor: remove Optional & nullopt #21415

fanquake · 2021-03-11T06:03:58Z

Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.

DrahtBot · 2021-03-11T06:58:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[rfc] add option to bypass contextual timelocks in testmempoolaccept? #21413 ([rfc] add option to bypass contextual timelocks in testmempoolaccept? by glozow)
Basic Taproot signing support for descriptor wallets #21365 (Basic Taproot signing support for descriptor wallets by sipa)
Implement BIP 370 PSBTv2 #21283 (Implement BIP 370 PSBTv2 by achow101)
A few descriptor improvements to prepare for Taproot support #21238 (A few descriptor improvements to prepare for Taproot support by sipa)
net processing: Extract addr send functionality into MaybeSendAddr() #21236 (Net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)
rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
Extend support for nested commands to bitcoin-cli #20273 (Extend support for nested commands to bitcoin-cli by jonasschnelli)
p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197 (p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack)
net: fix GetListenPort() to derive the proper port #20196 (net: fix GetListenPort() to derive the proper port by vasild)
[BIP 174] PSBT version, proprietary, and xpub fields #17034 ([BIP 174] PSBT version, proprietary, and xpub fields by achow101)

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.

jnewbery

ACK 8d2046678556294a97e2e9809e55c9ce19860569.

Change looks correct, but now that we're using std::optional rather than boost::optional, we can be a lot more idiomatic. std::nullopt almost never needs to be used explicitly - std::optional<> objects are default initialized to std::nullopt, std::nullopt can be initialized with empty braces { }, and the boolean operator can be used instead of explicit comparison with std::nullopt.

src/node/psbt.h

src/net.cpp

src/txmempool.cpp

src/validation.h

src/wallet/rpcdump.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

src/miner.cpp

JeremyRubin · 2021-03-11T17:58:30Z

might be worth changing the style guide -- I personally think that using nullopt is a good tradeoff between verbosity and legibility ie https://stackoverflow.com/questions/62933403/why-do-we-need-stdnullopt in many cases.

jonatack · 2021-03-11T18:05:18Z

might be worth changing the style guide -- I personally think that using nullopt is a good tradeoff between verbosity and legibility ie https://stackoverflow.com/questions/62933403/why-do-we-need-stdnullopt in many cases.

Similar questions in a couple places today: #21407 (comment)

practicalswift · 2021-03-11T22:12:32Z

Concept ACK

Thanks for removing old cruft!

glozow

why no s/#include <optional.h>/#include <optional> ?

-BEGIN VERIFY SCRIPT- git rm src/optional.h sed -i -e 's/Optional</std::optional</g' $(git grep -l 'Optional<' src) sed -i -e 's/{nullopt}/{std::nullopt}/g' $(git grep -l 'nullopt' src) sed -i -e 's/ nullopt;/ std::nullopt;/g' $(git grep -l 'nullopt' src) sed -i -e 's/ nullopt)/ std::nullopt)/g' $(git grep -l 'nullopt' src) sed -i -e 's/(nullopt)/(std::nullopt)/g' $(git grep -l 'nullopt' src) sed -i -e 's/ nullopt,/ std::nullopt,/g' $(git grep -l 'nullopt' src) sed -i -e 's/? nullopt :/? std::nullopt :/g' $(git grep -l 'nullopt' src) sed -i -e 's/: nullopt}/: std::nullopt}/g' $(git grep -l 'nullopt' src) sed -i -e '/optional.h \\/d' src/Makefile.am sed -i -e '/#include <optional.h>/d' src/test/fuzz/autofile.cpp src/test/fuzz/buffered_file.cpp src/test/fuzz/node_eviction.cpp sed -i -e 's/#include <optional.h>/#include <optional>/g' $(git grep -l '#include <optional.h>' src) -END VERIFY SCRIPT-

fanquake · 2021-03-15T04:03:16Z

Similar to #21404 I've left comments where new Optional<> usage is being introduced. i.e here, here, here etc.

why no s/#include <optional.h>/#include ?

It was originally this way, but the duplicate include linter was unhappy. This should now be fixed.

Have added some additional commits to take post-removal suggestions.

src/interfaces/chain.h

jnewbery

Just a couple more style comments

src/bench/wallet_balance.cpp

src/interfaces/chain.h

src/validation.h

fanquake · 2021-03-15T08:49:50Z

I've added new includes, sorted existing includes, removed a comment and realigned other comments.

jnewbery · 2021-03-15T09:53:27Z

ACK dca584e768

practicalswift · 2021-03-15T16:00:22Z

src/net.cpp

Nit: msg.reset(); is now used on L755. Was the intention to use it also on L750?

practicalswift · 2021-03-15T16:06:58Z

cr ACK dca584e7684c38afb9e5267104d0c0a008fd7477: patch looks correct. The AppVeyor failure looks spurious.

DrahtBot · 2021-03-15T16:48:45Z

🕵️ @achow101 @sipa have been requested to review this pull request as specified in the REVIEWERS file.

ryanofsky

Code review ACK dca584e7684c38afb9e5267104d0c0a008fd7477. Kill it with fire 🔥🧨🧑‍🚒

ryanofsky · 2021-03-15T19:15:25Z

src/miner.h

At first glance, I thought adding inline keyword here might cause separate m_last_block_num_txs and m_last_block_weight variable instances to exist in different translation units (different variables with the same name miner.cpp and rpc/mining.cpp that would lead to bugs), but apparently there is new linker magic in c++17 to prevent this!

✨ m a g i c ✨

Good to know this

(more background here: #21415 (comment))

laanwj · 2021-03-16T12:58:40Z

I personally think that using nullopt is a good tradeoff between verbosity and legibility ie https://stackoverflow.com/questions/62933403/why-do-we-need-stdnullopt in many cases.

I have the same opinion about this. Using std::optional is a no-brainer but replacing nullopt with {} doesn't always seem like a readability improvement to me. {} is this kind of wildcard that can be anything from an empty structure, a default initializer, etc, nullopt is type-safe and clear.

fanquake · 2021-03-17T07:05:10Z

I have the same opinion about this.

Ok. I've dropped f603e792c083fe9c9072dc3e753dcf9922583ee0.

practicalswift · 2021-03-17T08:28:20Z

cr ACK ebc4ab7: patch looks correct

jnewbery · 2021-03-17T09:04:40Z

utACK ebc4ab7

laanwj · 2021-03-17T11:17:10Z

Code review ACK ebc4ab7

jonatack

Posthumous ACK ebc4ab7

In bitcoin#21415 we decided to return `std::optional` rather than `{}` for uninitialized values. This PR repalces the two remaining usages of `{}` with `std::nullopt`. As a side-effect, this also quells the spurious GCC 10.2.x warning that we've had reported quite a few times. i.e bitcoin#21318, bitcoin#21248, bitcoin#20797. ```bash txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’: txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 898 | return {}; | ^ ```

5294f0d refactor: return std::nullopt instead of {} (fanquake) Pull request description: In #21415 [we decided](bitcoin/bitcoin#21415 (comment)) to return `std::optional` rather than `{}` for uninitialized values. This PR replaces the two remaining usages of `{}` with `std::nullopt`. As a side-effect, this also quells the spurious GCC 10.2.x warning that we've had reported quite a few times. i.e #21318, #21248, #20797. ```bash txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’: txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 898 | return {}; | ^ ``` ACKs for top commit: hebasto: ACK 5294f0d, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0

5294f0d refactor: return std::nullopt instead of {} (fanquake) Pull request description: In bitcoin#21415 [we decided](bitcoin#21415 (comment)) to return `std::optional` rather than `{}` for uninitialized values. This PR replaces the two remaining usages of `{}` with `std::nullopt`. As a side-effect, this also quells the spurious GCC 10.2.x warning that we've had reported quite a few times. i.e bitcoin#21318, bitcoin#21248, bitcoin#20797. ```bash txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’: txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 898 | return {}; | ^ ``` ACKs for top commit: hebasto: ACK 5294f0d, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0

fanquake added the Refactoring label Mar 11, 2021

fanquake force-pushed the remove_optional_wrapper branch 3 times, most recently from ee3dda0 to 8d20466 Compare March 11, 2021 06:34

jnewbery reviewed Mar 11, 2021

View reviewed changes

This was referenced Mar 11, 2021

[rfc] add option to bypass contextual timelocks in testmempoolaccept? #21413

Closed

Basic Taproot signing support for descriptor wallets #21365

Merged

Implement BIP 370 PSBTv2 #21283

Open

This was referenced Mar 11, 2021

A few descriptor improvements to prepare for Taproot support #21238

Merged

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

glozow reviewed Mar 12, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 12, 2021

[BIP 174] PSBT version, proprietary, and xpub fields #17034

Merged

fanquake force-pushed the remove_optional_wrapper branch from 8d20466 to ea889a6 Compare March 15, 2021 03:59

maflcko reviewed Mar 15, 2021

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

jnewbery reviewed Mar 15, 2021

View reviewed changes

src/bench/wallet_balance.cpp Outdated Show resolved Hide resolved

src/interfaces/chain.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

fanquake force-pushed the remove_optional_wrapper branch from ea889a6 to dca584e Compare March 15, 2021 08:48

practicalswift reviewed Mar 15, 2021

View reviewed changes

ryanofsky approved these changes Mar 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 15, 2021

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

refactor: post Optional<> removal cleanups

ebc4ab7

fanquake force-pushed the remove_optional_wrapper branch from dca584e to ebc4ab7 Compare March 17, 2021 07:04

laanwj merged commit a9d1b40 into bitcoin:master Mar 17, 2021

fanquake deleted the remove_optional_wrapper branch March 17, 2021 11:31

jonatack reviewed Mar 17, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 17, 2021

Merge bitcoin#21415: refactor: remove Optional & nullopt

3562cc7

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 17, 2021

Merge bitcoin#21415: refactor: remove Optional & nullopt

68a7fcd

fanquake mentioned this pull request Mar 22, 2021

refactor: return std::nullopt instead of {} #21498

Merged

theStack mentioned this pull request Mar 31, 2021

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

This was referenced Apr 12, 2021

Wallet passive startup #15719

Closed

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

refactor: remove Optional & nullopt #21415

refactor: remove Optional & nullopt #21415

Uh oh!

Conversation

fanquake commented Mar 11, 2021

Uh oh!

DrahtBot commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeremyRubin commented Mar 11, 2021

Uh oh!

jonatack commented Mar 11, 2021

Uh oh!

practicalswift commented Mar 11, 2021

Uh oh!

glozow left a comment • 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!

fanquake commented Mar 15, 2021

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanquake commented Mar 15, 2021

Uh oh!

jnewbery commented Mar 15, 2021

Uh oh!

practicalswift Mar 15, 2021 • 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!

practicalswift commented Mar 15, 2021

Uh oh!

DrahtBot commented Mar 15, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 16, 2021

Uh oh!

fanquake commented Mar 17, 2021

Uh oh!

practicalswift commented Mar 17, 2021

Uh oh!

jnewbery commented Mar 17, 2021

Uh oh!

laanwj commented Mar 17, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

DrahtBot commented Mar 11, 2021 •

edited

Loading

glozow left a comment •

edited

Loading

practicalswift Mar 15, 2021 •

edited

Loading