refactor: remove Optional & nullopt

fanquake commented at 6:03 am on March 11, 2021: member

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.

fanquake added the label Refactoring on Mar 11, 2021

fanquake force-pushed on Mar 11, 2021

DrahtBot commented at 6:58 am on March 11, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

in src/node/psbt.h:31 in 8d20466785 outdated

29-    Optional<CFeeRate> estimated_feerate;  //!< Estimated feerate (fee / weight) of the transaction
30-    Optional<CAmount> fee;                 //!< Amount of fee being paid by the transaction
31+    std::optional<size_t> estimated_vsize;      //!< Estimated weight of the transaction
32+    std::optional<CFeeRate> estimated_feerate;  //!< Estimated feerate (fee / weight) of the transaction
33+    std::optional<CAmount> fee;                 //!< Amount of fee being paid by the transaction
34     std::vector<PSBTInputAnalysis> inputs; //!< More information about the individual inputs of the transaction

jnewbery commented at 10:29 am on March 11, 2021:

You could realign these comments if you wanted to be nice.

MarcoFalke commented at 11:18 am on March 11, 2021:

run a scripted diff with clang format on the diff? (*hides)

in src/net.cpp:218 in 8d20466785 outdated

213@@ -215,7 +214,7 @@ Optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
214         return addrLocal;
215     }
216     // Address is unroutable. Don't advertise.
217-    return nullopt;
218+    return std::nullopt;

jnewbery commented at 10:37 am on March 11, 2021:

It may be slightly more idiomatic to replace all the return std::nullopt; with return {};

in src/net.cpp:750 in 8d20466785 outdated

745@@ -747,12 +746,12 @@ Optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::mic
746                  HexStr(hdr.pchChecksum),
747                  m_node_id);
748         out_err_raw_size = msg->m_raw_message_size;
749-        msg = nullopt;
750+        msg = std::nullopt;

jnewbery commented at 10:39 am on March 11, 2021:

Perhaps msg.reset() is more idiomatic.

in src/txmempool.cpp:896 in 8d20466785 outdated

893+std::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const
894 {
895     auto it = mapTx.find(txid);
896     if (it != mapTx.end()) return it;
897-    return Optional<txiter>{};
898+    return std::optional<txiter>{};

jnewbery commented at 10:41 am on March 11, 2021:

0    return {};

fanquake commented at 5:07 am on March 15, 2021:

This should also finally quell the -Wmaybe-uninitialized false positives we’ve seen. i.e #21318, #21248.

in src/validation.h:210 in 8d20466785 outdated

206@@ -208,7 +207,7 @@ struct MempoolAcceptResult {
207     /** Constructor for failure case */
208     explicit MempoolAcceptResult(TxValidationState state)
209         : m_result_type(ResultType::INVALID),
210-        m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {
211+        m_state(state), m_replaced_transactions(std::nullopt), m_base_fees(std::nullopt) {

jnewbery commented at 10:43 am on March 11, 2021:

No need for these initializers. std::optional default initializes to std::nullopt

in src/wallet/rpcdump.cpp:1791 in 8d20466785 outdated

1787@@ -1788,7 +1788,7 @@ RPCHelpMan listdescriptors()
1788         const bool active = active_spk_mans.count(desc_spk_man) != 0;
1789         spk.pushKV("active", active);
1790         const auto& type = wallet_descriptor.descriptor->GetOutputType();
1791-        if (active && type != nullopt) {
1792+        if (active && type != std::nullopt) {

jnewbery commented at 10:43 am on March 11, 2021:

More idiomatic:

0        if (active && type) {

in src/wallet/rpcwallet.cpp:221 in 8d20466785 outdated

217@@ -219,7 +218,7 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un
218         cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
219         if (override_min_fee) cc.fOverrideFeeRate = true;
220         // Default RBF to true for explicit fee_rate, if unset.
221-        if (cc.m_signal_bip125_rbf == nullopt) cc.m_signal_bip125_rbf = true;
222+        if (cc.m_signal_bip125_rbf == std::nullopt) cc.m_signal_bip125_rbf = true;

jnewbery commented at 10:44 am on March 11, 2021:

0        if (!cc.m_signal_bip125_rbf) cc.m_signal_bip125_rbf = true;

in src/wallet/wallet.cpp:89 in 8d20466785 outdated

82@@ -84,10 +83,10 @@ bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_nam
83 
84 static void UpdateWalletSetting(interfaces::Chain& chain,
85                                 const std::string& wallet_name,
86-                                Optional<bool> load_on_startup,
87+                                std::optional<bool> load_on_startup,
88                                 std::vector<bilingual_str>& warnings)
89 {
90-    if (load_on_startup == nullopt) return;
91+    if (load_on_startup == std::nullopt) return;

jnewbery commented at 10:45 am on March 11, 2021:

0    if (!load_on_startup) return;

in src/miner.cpp:99 in 8d20466785 outdated

 95@@ -96,8 +96,8 @@ void BlockAssembler::resetBlock()
 96     nFees = 0;
 97 }
 98 
 99-Optional<int64_t> BlockAssembler::m_last_block_num_txs{nullopt};
100-Optional<int64_t> BlockAssembler::m_last_block_weight{nullopt};
101+std::optional<int64_t> BlockAssembler::m_last_block_num_txs{std::nullopt};

jnewbery commented at 10:47 am on March 11, 2021:

Not required. std::optional<> will default initialize to std::nullopt. These lines can be removed entirely.

MarcoFalke commented at 11:16 am on March 11, 2021:

They are static, but this should still zero initialize: https://en.cppreference.com/w/cpp/language/zero_initialization

jnewbery commented at 1:45 pm on March 11, 2021:

Does this seem right to you, @MarcoFalke:

 0diff --git a/src/miner.cpp b/src/miner.cpp
 1index 56543e743e..5239836e0f 100644
 2--- a/src/miner.cpp
 3+++ b/src/miner.cpp
 4@@ -96,8 +96,8 @@ void BlockAssembler::resetBlock()
 5     nFees = 0;
 6 }
 7 
 8-std::optional<int64_t> BlockAssembler::m_last_block_num_txs{std::nullopt};
 9-std::optional<int64_t> BlockAssembler::m_last_block_weight{std::nullopt};
10+/* std::optional<int64_t> BlockAssembler::m_last_block_num_txs{std::nullopt}; */
11+/* std::optional<int64_t> BlockAssembler::m_last_block_weight{std::nullopt}; */
12 
13 std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
14 {
15diff --git a/src/miner.h b/src/miner.h
16index 286a8c693e..9dadbf1507 100644
17--- a/src/miner.h
18+++ b/src/miner.h
19@@ -159,8 +159,8 @@ public:
20     /** Construct a new block template with coinbase to scriptPubKeyIn */
21     std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
22 
23-    static std::optional<int64_t> m_last_block_num_txs;
24-    static std::optional<int64_t> m_last_block_weight;
25+    inline static std::optional<int64_t> m_last_block_num_txs{};
26+    inline static std::optional<int64_t> m_last_block_weight{};
27 
28 private:
29     // utility functions

MarcoFalke commented at 3:36 pm on March 11, 2021:

Is the inline needed? Any version that passes mining_basic.py under valgrind should be correct. See fa178a6385bf300499fb18940051fc4142fb5b6b

jnewbery commented at 5:13 pm on March 11, 2021:

I’m pretty sure inline is required. Here’s what you get if the variables aren’t inline:

0In file included from miner.cpp:6:
1./miner.h:162:35: error: non-const static data member must be initialized out of line
2    static std::optional<int64_t> m_last_block_num_txs{};
3                                  ^~~~~~~~~~~~~~~~~~~~~~
4./miner.h:163:35: error: non-const static data member must be initialized out of line
5    static std::optional<int64_t> m_last_block_weight{};
6                                  ^~~~~~~~~~~~~~~~~~~~~
72 errors generated.

Inline member variables have been possible since c++17: https://www.codingame.com/playgrounds/2205/7-features-of-c17-that-will-simplify-your-code/inline-variables.

MarcoFalke commented at 8:02 pm on March 11, 2021:

thanks. Seems fine to change this to inline static initialization.

jnewbery commented at 10:49 am on March 11, 2021: member

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.

JeremyRubin commented at 5:58 pm on March 11, 2021: contributor

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 commented at 6:05 pm on March 11, 2021: member

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

practicalswift commented at 10:12 pm on March 11, 2021: contributor

Concept ACK

Thanks for removing old cruft!

glozow commented at 0:02 am on March 12, 2021: member

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

scripted-diff: remove Optional & nullopt

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

57e980d13c

fanquake force-pushed on Mar 15, 2021

fanquake commented at 4:03 am on March 15, 2021: member

[Similar to #21404](/bitcoin-bitcoin/21404/#issuecomment-797163202) 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.

in src/bench/wallet_balance.cpp:8 in ea889a6351 outdated

4@@ -5,7 +5,7 @@
5 #include <bench/bench.h>
6 #include <interfaces/chain.h>
7 #include <node/context.h>
8-#include <optional.h>
9+#include <optional>

jnewbery commented at 7:59 am on March 15, 2021:

Are you able to separate the standard library includes from the local project includes?

in src/interfaces/chain.h:8 in ea889a6351 outdated

4@@ -5,7 +5,7 @@
5 #ifndef BITCOIN_INTERFACES_CHAIN_H
6 #define BITCOIN_INTERFACES_CHAIN_H
7 
8-#include <optional.h>               // For Optional and nullopt
9+#include <optional>               // For Optional and nullopt

jnewbery commented at 8:00 am on March 15, 2021:

Remove comment (and separate from project includes)

MarcoFalke commented at 8:08 am on March 15, 2021:

style nit: :eye: Comment wrong. Also, would be nice if you sorted the includes

in src/validation.h:209 in ea889a6351 outdated

205@@ -206,7 +206,7 @@ struct MempoolAcceptResult {
206     /** Constructor for failure case */
207     explicit MempoolAcceptResult(TxValidationState state)
208         : m_result_type(ResultType::INVALID),
209-        m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {
210+        m_state(state), m_replaced_transactions(), m_base_fees() {

jnewbery commented at 8:16 am on March 15, 2021:

Just remove these from the initializer list entirely:

0    explicit MempoolAcceptResult(TxValidationState state)
1        : m_result_type(ResultType::INVALID), m_state(state)
2    { Assume(!state.IsValid()); } // Can be invalid or error

jnewbery commented at 8:16 am on March 15, 2021: member

Just a couple more style comments

fanquake force-pushed on Mar 15, 2021

fanquake commented at 8:49 am on March 15, 2021: member

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

jnewbery commented at 9:53 am on March 15, 2021: member

ACK dca584e768

in src/net.cpp:750 in dca584e768 outdated

746@@ -747,12 +747,12 @@ Optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::mic
747                  HexStr(hdr.pchChecksum),
748                  m_node_id);
749         out_err_raw_size = msg->m_raw_message_size;
750-        msg = nullopt;
751+        msg = std::nullopt;

practicalswift commented at 4:00 pm on March 15, 2021:

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

practicalswift commented at 4:06 pm on March 15, 2021: contributor

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

DrahtBot commented at 4:48 pm on March 15, 2021: member

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

in src/miner.h:164 in dca584e768 outdated

159@@ -160,8 +160,8 @@ class BlockAssembler
160     /** Construct a new block template with coinbase to scriptPubKeyIn */
161     std::unique_ptr<CBlockTemplate> CreateNewBlock(CChainState& chainstate, const CScript& scriptPubKeyIn);
162 
163-    static std::optional<int64_t> m_last_block_num_txs;
164-    static std::optional<int64_t> m_last_block_weight;
165+    inline static std::optional<int64_t> m_last_block_num_txs{};
166+    inline static std::optional<int64_t> m_last_block_weight{};

ryanofsky commented at 7:15 pm on March 15, 2021:

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!

jnewbery commented at 7:30 pm on March 15, 2021:

✨ m a g i c ✨

laanwj commented at 12:55 pm on March 16, 2021:

Good to know this

jnewbery commented at 1:24 pm on March 16, 2021:

(more background here: #21415 (review))

ryanofsky approved

ryanofsky commented at 7:18 pm on March 15, 2021: member

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

laanwj commented at 12:58 pm on March 16, 2021: member

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.

refactor: post Optional<> removal cleanups ebc4ab721b

fanquake force-pushed on Mar 17, 2021

fanquake commented at 7:05 am on March 17, 2021: member

I have the same opinion about this.

Ok. I’ve dropped f603e792c083fe9c9072dc3e753dcf9922583ee0.

practicalswift commented at 8:28 am on March 17, 2021: contributor

cr ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6: patch looks correct

jnewbery commented at 9:04 am on March 17, 2021: member

utACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6

laanwj commented at 11:17 am on March 17, 2021: member

Code review ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6

laanwj merged this on Mar 17, 2021

laanwj closed this on Mar 17, 2021

fanquake deleted the branch on Mar 17, 2021

jonatack commented at 2:31 pm on March 17, 2021: member

Posthumous ACK ebc4ab721b037

sidhujag referenced this in commit 3562cc7b2a on Mar 17, 2021

sidhujag referenced this in commit 68a7fcd8bb on Mar 17, 2021

fanquake referenced this in commit 5294f0d5a9 on Mar 22, 2021

MarcoFalke referenced this in commit 786654aa5e on Mar 22, 2021

sidhujag referenced this in commit 9f12fb3d2e on Mar 22, 2021

DrahtBot locked this on Aug 18, 2022

refactor: remove Optional & nullopt #21415

Conflicts