refactor: remove Optional & nullopt #21415

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:remove_optional_wrapper changing 51 files +184 −194
  1. 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.

  2. fanquake added the label Refactoring on Mar 11, 2021
  3. fanquake force-pushed on Mar 11, 2021
  4. fanquake force-pushed on Mar 11, 2021
  5. fanquake force-pushed on Mar 11, 2021
  6. DrahtBot commented at 6:58 AM on March 11, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

  8. 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 {};

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

  10. 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: 
        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.

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

  12. 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:

            if (active && type) {
  13. 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: 
            if (!cc.m_signal_bip125_rbf) cc.m_signal_bip125_rbf = true;
  14. 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: 
        if (!load_on_startup) return;
  15. 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:

    diff --git a/src/miner.cpp b/src/miner.cpp
index 56543e743e..5239836e0f 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -96,8 +96,8 @@ void BlockAssembler::resetBlock()
     nFees = 0;
 }
 
-std::optional<int64_t> BlockAssembler::m_last_block_num_txs{std::nullopt};
-std::optional<int64_t> BlockAssembler::m_last_block_weight{std::nullopt};
+/* std::optional<int64_t> BlockAssembler::m_last_block_num_txs{std::nullopt}; */
+/* std::optional<int64_t> BlockAssembler::m_last_block_weight{std::nullopt}; */
 
 std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
 {
diff --git a/src/miner.h b/src/miner.h
index 286a8c693e..9dadbf1507 100644
--- a/src/miner.h
+++ b/src/miner.h
@@ -159,8 +159,8 @@ public:
     /** Construct a new block template with coinbase to scriptPubKeyIn */
     std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
 
-    static std::optional<int64_t> m_last_block_num_txs;
-    static std::optional<int64_t> m_last_block_weight;
+    inline static std::optional<int64_t> m_last_block_num_txs{};
+    inline static std::optional<int64_t> m_last_block_weight{};
 
 private:
     // 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:

    In file included from miner.cpp:6:
./miner.h:162:35: error: non-const static data member must be initialized out of line
    static std::optional<int64_t> m_last_block_num_txs{};
                                  ^~~~~~~~~~~~~~~~~~~~~~
./miner.h:163:35: error: non-const static data member must be initialized out of line
    static std::optional<int64_t> m_last_block_weight{};
                                  ^~~~~~~~~~~~~~~~~~~~~
2 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.

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

  17. DrahtBot cross-referenced this on Mar 11, 2021 from issue [rfc] add option to bypass contextual timelocks in testmempoolaccept? by glozow
  18. DrahtBot cross-referenced this on Mar 11, 2021 from issue Basic Taproot signing support for descriptor wallets by sipa
  19. DrahtBot cross-referenced this on Mar 11, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  20. 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.

  21. DrahtBot cross-referenced this on Mar 11, 2021 from issue A few descriptor improvements to prepare for Taproot support by sipa
  22. DrahtBot cross-referenced this on Mar 11, 2021 from issue net processing: Extract `addr` send functionality into MaybeSendAddr() by jnewbery
  23. jonatack commented at 6:05 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.

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

  24. DrahtBot cross-referenced this on Mar 11, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky
  25. DrahtBot cross-referenced this on Mar 11, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  26. DrahtBot cross-referenced this on Mar 11, 2021 from issue rpc/validation: enable packages through testmempoolaccept by glozow
  27. DrahtBot cross-referenced this on Mar 11, 2021 from issue Extend support for nested commands to bitcoin-cli by jonasschnelli
  28. DrahtBot cross-referenced this on Mar 11, 2021 from issue p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack
  29. DrahtBot cross-referenced this on Mar 11, 2021 from issue net: fix GetListenPort() to derive the proper port by vasild
  30. practicalswift commented at 10:12 PM on March 11, 2021: contributor

    Concept ACK

    Thanks for removing old cruft!

  31. glozow commented at 12:02 AM on March 12, 2021: member

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

  32. DrahtBot cross-referenced this on Mar 12, 2021 from issue [BIP 174] PSBT version, proprietary, and xpub fields by achow101
  33. 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
  34. fanquake force-pushed on Mar 15, 2021
  35. fanquake commented at 4:03 AM on March 15, 2021: member

    Similar to [#21404](/github-metadata-backup-bitcoin-bitcoin/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 <optional> ?

    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.

  36. 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?

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

  38. 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:

        explicit MempoolAcceptResult(TxValidationState state)
        : m_result_type(ResultType::INVALID), m_state(state)
    { Assume(!state.IsValid()); } // Can be invalid or error
  39. jnewbery commented at 8:16 AM on March 15, 2021: member

    Just a couple more style comments

  40. fanquake force-pushed on Mar 15, 2021
  41. 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.

  42. jnewbery commented at 9:53 AM on March 15, 2021: member

    ACK dca584e768

  43. 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?

  44. practicalswift commented at 4:06 PM on March 15, 2021: contributor

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

  45. DrahtBot commented at 4:48 PM on March 15, 2021: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

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

  46. 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))

  47. ryanofsky approved
  48. ryanofsky commented at 7:18 PM on March 15, 2021: contributor

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

  49. DrahtBot cross-referenced this on Mar 15, 2021 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
  50. 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.

  51. refactor: post Optional<> removal cleanups ebc4ab721b
  52. fanquake force-pushed on Mar 17, 2021
  53. fanquake commented at 7:05 AM on March 17, 2021: member

    I have the same opinion about this.

    Ok. I've dropped f603e792c083fe9c9072dc3e753dcf9922583ee0.

  54. practicalswift commented at 8:28 AM on March 17, 2021: contributor

    cr ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6: patch looks correct

  55. jnewbery commented at 9:04 AM on March 17, 2021: member

    utACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6

  56. laanwj commented at 11:17 AM on March 17, 2021: member

    Code review ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6

  57. laanwj merged this on Mar 17, 2021
  58. laanwj closed this on Mar 17, 2021
  59. fanquake deleted the branch on Mar 17, 2021
  60. jonatack commented at 2:31 PM on March 17, 2021: contributor

    Posthumous ACK ebc4ab721b037

  61. sidhujag referenced this in commit 3562cc7b2a on Mar 17, 2021
  62. sidhujag referenced this in commit 68a7fcd8bb on Mar 17, 2021
  63. fanquake referenced this in commit 5294f0d5a9 on Mar 22, 2021
  64. fanquake cross-referenced this on Mar 22, 2021 from issue refactor: return std::nullopt instead of {} by fanquake
  65. MarcoFalke referenced this in commit 786654aa5e on Mar 22, 2021
  66. sidhujag referenced this in commit 9f12fb3d2e on Mar 22, 2021
  67. theStack cross-referenced this on Mar 31, 2021 from issue wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack
  68. ryanofsky cross-referenced this on Apr 12, 2021 from issue Wallet passive startup by ryanofsky
  69. ryanofsky cross-referenced this on Apr 12, 2021 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky
  70. bitcoin locked this on Aug 18, 2022
Contributors
fanquakeDrahtBotjnewberyMarcoFalkeJeremyRubinjonatackpracticalswiftglozowryanofskylaanwj
Labels
Refactoring
Linked (view graph)
#15719 Wallet passive startup#17783 common: Disallow calling IsArgSet() on ALLOW_LIST options#20640 wallet, refactor: return out-params of CreateTransaction() as optional struct#21248 validation: -Wmaybe-uninitialized compiler warning thrown by CTxMempool::GetIterSet #21318 False positive -Wmaybe-uninitialized warning of GCC 10.2#21404 refactor: Remove MakeUnique<T>()#21407 i2p: limit the size of incoming messages#21498 refactor: return std::nullopt instead of {}
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC