Replace boost::optional with std::optional #20671

Now that we can use std::optional from the vanilla standard library, drop the third-party boost dependency

Nice, concept ACK, this is a surprisingly small patch.

Concept ACK.

Concept ACK -- nice

Concept ACK.

#19806 could be rebased on this to remove the need for additional boost dependencies: #19806 (review). @MarcoFalke do you still consider #19426 a prerequisite for this? (https://github.com/bitcoin/bitcoin/pull/19183#issuecomment-733637916)

Concept ACK: nice to get rid of old cruft

@MarcoFalke do you still consider #19426 a prerequisite for this? (#19183 (comment))

I was hoping to get that in first, so that the first commit is easier to review. Though, it is not a strict requirement.

References for the third commit:

https://www.boost.org/doc/libs/1_75_0/libs/optional/doc/html/boost_optional/reference/header__boost_optional_optional_hpp_/detailed_semantics___optional_values.html

https://en.cppreference.com/w/cpp/utility/optional

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

• #19806 (validation: UTXO snapshot activation by jamesob)
• #19426 (refactor: Change * to & in MutableTransactionSignatureCreator by MarcoFalke)
• #19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)

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.

Concept ACK.

Replace get_ptr() with &value() in UpdatePSBTOutput This is safe because a value is always contained in this optional

What do you think about explicitly stating that assumption at the top of the function:

--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -207,7 +207,11 @@ size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt) {
 
 void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)
 {
-    const CTxOut& out = psbt.tx->vout.at(index);
+    // tx must be filled before updating this PSBT.
+    assert(psbt.tx.has_value());
+    CMutableTransaction& tx = psbt.tx.value();
+
+    const CTxOut& out = tx.vout.at(index);
     PSBTOutput& psbt_out = psbt.outputs.at(index);
 
     // Fill a SignatureData with output info
@@ -217,7 +221,7 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
     // Construct a would-be spend of this output, to update sigdata with.
     // Note that ProduceSignature is used to fill in metadata (not actual signatures),
     // so provider does not need to provide any private keys (it can be a HidingSigningProvider).
-    MutableTransactionSignatureCreator creator(&psbt.tx.value(), /* index */ 0, out.nValue, SIGHASH_ALL);
+    MutableTransactionSignatureCreator creator(&tx, /* index */ 0, out.nValue, SIGHASH_ALL);

cr ACK fa4435e22f78f632a455016ce00a357009aac059: patch looks correct!

ACK fa4435e22f78f632a455016ce00a357009aac059, I have reviewed the code and it looks OK, I agree it can be merged.

code review ACK fa4435e22f78f632a455016ce00a357009aac059

Since this PR many of the functional tests fail with --valgrind. This feels like a compiler bug. It doesn't happen with --enable-debug and stops happening when I try to add trace code or extra conditonals.

e.g. on 68c7acf6bb7733261865afdcc05ee687ef75489d

19:25:55 -bash@camus ~/code/bitcoin/bitcoin/master 68c7acf6bb7$ ./test/functional/interface_bitcoin_cli.py --valgrind
2021-08-03T19:25:55.328000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_h1zfvbq7
2021-08-03T19:26:04.859000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
    <python noise redacted, just saying "stderr was not empty">
AssertionError: Unexpected stderr ==1787451== Thread 19 b-httpworker.0:
==1787451== Conditional jump or move depends on uninitialised value(s)
==1787451==    at 0x5074E0: CWallet::ScanForWalletTransactions(uint256 const&, int, std::optional<int>, WalletRescanReserver const&, bool) (wallet.cpp:1805)
==1787451==    by 0x50809C: CWallet::RescanFromTime(long, WalletRescanReserver const&, bool) (wallet.cpp:1708)
==1787451==    by 0x5581DB: RescanWallet(CWallet&, WalletRescanReserver const&, long, bool) (rpcdump.cpp:85)
==1787451==    by 0x57537E: importprivkey()::{lambda(RPCHelpMan const&, JSONRPCRequest const&)#1}::operator()(RPCHelpMan const&, JSONRPCRequest const&) const [clone .constprop.0] (rpcdump.cpp:188)
==1787451==    by 0x575694: __invoke_impl<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&> (invoke.h:61)
==1787451==    by 0x575694: __invoke_r<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&> (invoke.h:116)
==1787451==    by 0x575694: std::_Function_handler<UniValue (RPCHelpMan const&, JSONRPCRequest const&), importprivkey()::{lambda(RPCHelpMan const&, JSONRPCRequest const&)#1}>::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) (std_function.h:292)
==1787451==    by 0x2D4A9E: operator() (std_function.h:560)
==1787451==    by 0x2D4A9E: HandleRequest (util.h:343)
==1787451==    by 0x2D4A9E: CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)(), std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const [clone .isra.0] (server.h:110)
==1787451==    by 0x43C473: operator() (std_function.h:560)
==1787451==    by 0x43C473: wallet::(anonymous namespace)::WalletClientImpl::registerRpcs()::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const [clone .isra.0] (interfaces.cpp:517)
==1787451==    by 0x251DE4: operator() (std_function.h:560)
==1787451==    by 0x251DE4: operator() (interfaces.cpp:382)
==1787451==    by 0x251DE4: __invoke_impl<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool> (invoke.h:61)
==1787451==    by 0x251DE4: __invoke_r<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool> (invoke.h:114)
==1787451==    by 0x251DE4: std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(CRPCCommand const&)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) (std_function.h:291)
==1787451==    by 0x352364: operator() (std_function.h:560)
==1787451==    by 0x352364: ExecuteCommand (server.cpp:466)
==1787451==    by 0x352364: CRPCTable::execute(JSONRPCRequest const&) const (server.cpp:449)
==1787451==    by 0x402AB9: HTTPReq_JSONRPC(util::Ref const&, HTTPRequest*) (httprpc.cpp:201)
==1787451==    by 0x40ED8C: operator() (std_function.h:560)
==1787451==    by 0x40ED8C: operator() (httpserver.cpp:49)
==1787451==    by 0x40ED8C: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:108)
==1787451==    by 0x40A7C7: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:336)
==1787451== 
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:_ZN7CWallet25ScanForWalletTransactionsERK7uint256iSt8optionalIiERK20WalletRescanReserverb
   fun:_ZN7CWallet14RescanFromTimeElRK20WalletRescanReserverb
   fun:_ZL12RescanWalletR7CWalletRK20WalletRescanReserverlb
   fun:_ZZ13importprivkeyvENKUlRK10RPCHelpManRK14JSONRPCRequestE_clES1_S4_.constprop.0
   fun:__invoke_impl<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&>
   fun:__invoke_r<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&>
   fun:_ZNSt17_Function_handlerIF8UniValueRK10RPCHelpManRK14JSONRPCRequestEZ13importprivkeyvEUlS3_S6_E_E9_M_invokeERKSt9_Any_dataS3_S6_
   fun:operator()
   fun:HandleRequest
   fun:_ZZN11CRPCCommandC4ENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES5_PF10RPCHelpManvESt6vectorIS5_SaIS5_EEENKUlRK14JSONRPCRequestR8UniValuebE_clESE_SG_b.isra.0
   fun:operator()
   fun:_ZZN6wallet12_GLOBAL__N_116WalletClientImpl12registerRpcsEvENKUlRK14JSONRPCRequestR8UniValuebE_clES4_S6_b.isra.0
   fun:operator()
   fun:operator()
   fun:__invoke_impl<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool>
   fun:__invoke_r<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool>
   fun:_ZNSt17_Function_handlerIFbRK14JSONRPCRequestR8UniValuebEZN4node12_GLOBAL__N_114RpcHandlerImplC4ERK11CRPCCommandEUlS2_S4_bE_E9_M_invokeERKSt9_Any_dataS2_S4_Ob
   fun:operator()
   fun:ExecuteCommand
   fun:_ZNK9CRPCTable7executeERK14JSONRPCRequest
   fun:_ZL15HTTPReq_JSONRPCRKN4util3RefEP11HTTPRequest
   fun:operator()
   fun:operator()
   fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
   fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
}
==1787451== 
==1787451== Exit program on first error (--exit-on-first-error=yes) != 
[node 0] Cleaning up leftover process

compiler version

19:29:11 -bash@camus ~/code/bitcoin/bitcoin/master 68c7acf6bb7$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --with-isl --with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit --enable-cet=auto --enable-checking=release --enable-clocale=gnu --enable-default-pie --enable-default-ssp --enable-gnu-indirect-function --enable-gnu-unique-object --enable-install-libiberty --enable-linker-build-id --enable-lto --enable-multilib --enable-plugin --enable-shared --enable-threads=posix --disable-libssp --disable-libstdcxx-pch --disable-libunwind-exceptions --disable-werror gdc_include_dir=/usr/include/dlang/gdc
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.1.0 (GCC) 

I think it is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635

See also #18785, an unrelated but similar issue