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

maflcko · 2020-12-16T11:10:53Z

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

laanwj · 2020-12-16T11:14:49Z

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

hebasto · 2020-12-16T11:21:10Z

Concept ACK.

jonatack · 2020-12-16T11:34:13Z

Concept ACK -- nice

jnewbery · 2020-12-16T11:34:42Z

Concept ACK.

#19806 could be rebased on this to remove the need for additional boost dependencies: #19806 (comment).

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

practicalswift · 2020-12-16T11:44:23Z

Concept ACK: nice to get rid of old cruft

maflcko · 2020-12-16T11:53:33Z

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

maflcko · 2020-12-16T12:07:40Z

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

DrahtBot · 2020-12-16T16:54:42Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

validation: UTXO snapshot activation #19806 (validation: UTXO snapshot activation by jamesob)
refactor: Change * to & in MutableTransactionSignatureCreator #19426 (refactor: Change * to & in MutableTransactionSignatureCreator by MarcoFalke)
[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #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.

promag · 2020-12-18T09:50:28Z

Concept ACK.

src/optional.h

jnewbery · 2020-12-18T11:55:24Z

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

This was previously done implicitly in boost::optional by BOOST_ASSERT. Also, it was checked at runtime by valgrind if for some reason the assert was disabled. std::optional dereference won't assert, so add the Assert here explicitly. The explicit Assert also helps to document the code better.

The only use was to work around a compiler warning in an ancient compiler, which we no longer support.

practicalswift · 2020-12-19T09:26:34Z

cr ACK fa4435e: patch looks correct!

hebasto

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

src/psbt.cpp

laanwj · 2020-12-21T12:10:01Z

code review ACK fa4435e

fa4435e Replace boost::optional with std::optional (MarcoFalke) fa7e803 Remove unused MakeOptional (MarcoFalke) fadd402 psbt: Assert that tx has a value in UpdatePSBTOutput (MarcoFalke) Pull request description: Now that we can use std::optional from the vanilla standard library, drop the third-party boost dependency ACKs for top commit: practicalswift: cr ACK fa4435e: patch looks correct! laanwj: code review ACK fa4435e hebasto: ACK fa4435e, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 50c5a1a130cac65e043e0177ba5b009fc2ba09343af4e23322ff2eb32184a55f8f2dea66e7a1b9d9acf56bc164eef4e47448750549a07f3b661199ac9bf9afef

apoelstra · 2021-08-03T19:28:00Z

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

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)

apoelstra · 2021-08-03T21:05:39Z

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

maflcko · 2021-08-04T05:43:35Z

See also #18785, an unrelated but similar issue

backport: bitcoin#15141, bitcoin#15437, bitcoin#16240, bitcoin#18923, bitcoin#19219, bitcoin#19277, bitcoin#20016, bitcoin#20671 (auxiliary backports)

DrahtBot added RPC/REST/ZMQ Wallet labels Dec 16, 2020

maflcko added Refactoring and removed RPC/REST/ZMQ Wallet labels Dec 16, 2020

This was referenced Dec 16, 2020

validation: UTXO snapshot activation #19806

Merged

refactor: Change * to & in MutableTransactionSignatureCreator #19426

Merged

[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #19183

Closed

promag reviewed Dec 18, 2020

View reviewed changes

src/optional.h Show resolved Hide resolved

maflcko force-pushed the 2012-stdOpt branch from fa94793 to fa06db6 Compare December 19, 2020 08:40

MarcoFalke added 3 commits December 19, 2020 09:45

Remove unused MakeOptional

fa7e803

The only use was to work around a compiler warning in an ancient compiler, which we no longer support.

Replace boost::optional with std::optional

fa4435e

maflcko force-pushed the 2012-stdOpt branch from fa06db6 to fa4435e Compare December 19, 2020 08:47

hebasto approved these changes Dec 19, 2020

View reviewed changes

src/psbt.cpp Show resolved Hide resolved

laanwj merged commit 68c7acf into bitcoin:master Dec 21, 2020

maflcko deleted the 2012-stdOpt branch December 21, 2020 12:23

dhruv mentioned this pull request Jan 4, 2021

rpc: simpler setban and new ban manipulation commands #19825

Closed

ryanofsky mentioned this pull request Jan 29, 2021

Multiprocess bitcoin #10102

Draft

Bushstar mentioned this pull request Sep 9, 2021

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

kwvg added a commit to kwvg/dash that referenced this pull request May 7, 2022

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

c1814a9

kwvg added a commit to kwvg/dash that referenced this pull request May 13, 2022

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

f9c3c38

kwvg added a commit to kwvg/dash that referenced this pull request May 17, 2022

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

22c2c94

kwvg added a commit to kwvg/dash that referenced this pull request May 17, 2022

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

4e438eb

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2022

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

8fa55be

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2022

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

e3217fc

kwvg added a commit to kwvg/dash that referenced this pull request Jun 11, 2022

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

c1c205b

kwvg added a commit to kwvg/dash that referenced this pull request Jun 11, 2022

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

bfcb647

kwvg added a commit to kwvg/dash that referenced this pull request Jun 12, 2022

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

93a20f0

kwvg added a commit to kwvg/dash that referenced this pull request Jun 12, 2022

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

f6a09a8

kwvg added a commit to kwvg/dash that referenced this pull request Jun 12, 2022

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

a52815e

kwvg added a commit to kwvg/dash that referenced this pull request Jun 12, 2022

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

bac909d

kwvg added a commit to kwvg/dash that referenced this pull request Jun 12, 2022

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

aa8c497

kwvg added a commit to kwvg/dash that referenced this pull request Jun 13, 2022

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

a9d8c22

kwvg added a commit to kwvg/dash that referenced this pull request Jun 14, 2022

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

bcac7ce

kwvg added a commit to kwvg/dash that referenced this pull request Jun 18, 2022

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

a52dd8c

kwvg added a commit to kwvg/dash that referenced this pull request Jun 23, 2022

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

0edb24b

kwvg added a commit to kwvg/dash that referenced this pull request Jul 2, 2022

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

e235d83

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

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

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

Uh oh!

Conversation

maflcko commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 16, 2020

Uh oh!

hebasto commented Dec 16, 2020

Uh oh!

jonatack commented Dec 16, 2020

Uh oh!

jnewbery commented Dec 16, 2020

Uh oh!

practicalswift commented Dec 16, 2020

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

DrahtBot commented Dec 16, 2020

Conflicts

Uh oh!

promag commented Dec 18, 2020

Uh oh!

Uh oh!

jnewbery commented Dec 18, 2020

Uh oh!

practicalswift commented Dec 19, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Dec 21, 2020

Uh oh!

apoelstra commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Aug 3, 2021

Uh oh!

maflcko commented Aug 4, 2021

Uh oh!

Uh oh!

maflcko commented Dec 16, 2020 •

edited

Loading

apoelstra commented Aug 3, 2021 •

edited

Loading