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

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2012-stdOpt changing 8 files +20 −26
  1. MarcoFalke commented at 11:10 am on December 16, 2020: member
    Now that we can use std::optional from the vanilla standard library, drop the third-party boost dependency
  2. laanwj commented at 11:14 am on December 16, 2020: member
    Nice, concept ACK, this is a surprisingly small patch.
  3. hebasto commented at 11:21 am on December 16, 2020: member
    Concept ACK.
  4. jonatack commented at 11:34 am on December 16, 2020: member
    Concept ACK – nice
  5. jnewbery commented at 11:34 am on December 16, 2020: member

    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)

  6. DrahtBot added the label RPC/REST/ZMQ on Dec 16, 2020
  7. DrahtBot added the label Wallet on Dec 16, 2020
  8. practicalswift commented at 11:44 am on December 16, 2020: contributor
    Concept ACK: nice to get rid of old cruft
  9. MarcoFalke commented at 11:53 am on December 16, 2020: member

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

  11. MarcoFalke added the label Refactoring on Dec 16, 2020
  12. MarcoFalke removed the label RPC/REST/ZMQ on Dec 16, 2020
  13. MarcoFalke removed the label Wallet on Dec 16, 2020
  14. DrahtBot commented at 4:54 pm on December 16, 2020: 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:

    • #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.

  15. promag commented at 9:50 am on December 18, 2020: member
    Concept ACK.
  16. in src/optional.h:12 in fa94793194 outdated 
     9 #include <utility>
10 
11-#include <boost/optional.hpp>
12-
13 //! Substitute for C++17 std::optional
14+//! DEPRECATED use std::optional in new code.
    promag commented at 10:00 am on December 18, 2020:
    To avoid conflicts with other pulls or backports? If so it could mention an acceptable time to remove this alias.
    MarcoFalke commented at 10:29 am on December 18, 2020:
    I’d say when 0.21.1 is out or when 0.22 is about to get branched off, whichever happens first.
    promag commented at 11:08 am on December 18, 2020:
    Sounds good to me.
  17. jnewbery commented at 11:55 am on December 18, 2020: member

    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:

     0--- a/src/psbt.cpp
 1+++ b/src/psbt.cpp
 2@@ -207,7 +207,11 @@ size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt) {
 3 
 4 void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)
 5 {
 6-    const CTxOut& out = psbt.tx->vout.at(index);
 7+    // tx must be filled before updating this PSBT.
 8+    assert(psbt.tx.has_value());
 9+    CMutableTransaction& tx = psbt.tx.value();
10+
11+    const CTxOut& out = tx.vout.at(index);
12     PSBTOutput& psbt_out = psbt.outputs.at(index);
13 
14     // Fill a SignatureData with output info
15@@ -217,7 +221,7 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
16     // Construct a would-be spend of this output, to update sigdata with.
17     // Note that ProduceSignature is used to fill in metadata (not actual signatures),
18     // so provider does not need to provide any private keys (it can be a HidingSigningProvider).
19-    MutableTransactionSignatureCreator creator(&psbt.tx.value(), /* index */ 0, out.nValue, SIGHASH_ALL);
20+    MutableTransactionSignatureCreator creator(&tx, /* index */ 0, out.nValue, SIGHASH_ALL);
  18. MarcoFalke force-pushed on Dec 19, 2020
  19. psbt: Assert that tx has a value in UpdatePSBTOutput 
    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.
    fadd4029dc
  20. Remove unused MakeOptional 
    The only use was to work around a compiler warning in an ancient
compiler, which we no longer support.
    fa7e803f3e
  21. Replace boost::optional with std::optional fa4435e22f
  22. MarcoFalke force-pushed on Dec 19, 2020
  23. practicalswift commented at 9:26 am on December 19, 2020: contributor
    cr ACK fa4435e22f78f632a455016ce00a357009aac059: patch looks correct!
  24. in src/psbt.cpp:212 in fa4435e22f 
    208@@ -207,7 +209,8 @@ size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt) {
209 
210 void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)
211 {
212-    const CTxOut& out = psbt.tx->vout.at(index);
213+    CMutableTransaction& tx = *Assert(psbt.tx);
    hebasto commented at 12:07 pm on December 19, 2020:

    nit: to be explicit

    0    CMutableTransaction& tx = *Assert(psbt.tx.has_value());

    as were suggested by @jnewbery.

    MarcoFalke commented at 12:12 pm on December 19, 2020:
    That wouldn’t compile. Also, what is the benefit of it?
    hebasto commented at 12:21 pm on December 19, 2020:

    That wouldn’t compile.

    Indeed :man_facepalming:

    Sorry for noise.

  25. hebasto approved
  26. hebasto commented at 12:08 pm on December 19, 2020: member
    ACK fa4435e22f78f632a455016ce00a357009aac059, I have reviewed the code and it looks OK, I agree it can be merged.
  27. laanwj commented at 12:10 pm on December 21, 2020: member
    code review ACK fa4435e22f78f632a455016ce00a357009aac059
  28. laanwj merged this on Dec 21, 2020
  29. laanwj closed this on Dec 21, 2020
  30. MarcoFalke deleted the branch on Dec 21, 2020
  31. sidhujag referenced this in commit f916304b3e on Dec 21, 2020
  32. apoelstra commented at 7:28 pm on August 3, 2021: contributor

    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

     019:25:55 -bash@camus ~/code/bitcoin/bitcoin/master 68c7acf6bb7$ ./test/functional/interface_bitcoin_cli.py --valgrind
 12021-08-03T19:25:55.328000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_h1zfvbq7
 22021-08-03T19:26:04.859000Z TestFramework (ERROR): Unexpected exception caught during testing
 3Traceback (most recent call last):
 4    <python noise redacted, just saying "stderr was not empty">
 5AssertionError: Unexpected stderr ==1787451== Thread 19 b-httpworker.0:
 6==1787451== Conditional jump or move depends on uninitialised value(s)
 7==1787451==    at 0x5074E0: CWallet::ScanForWalletTransactions(uint256 const&, int, std::optional<int>, WalletRescanReserver const&, bool) (wallet.cpp:1805)
 8==1787451==    by 0x50809C: CWallet::RescanFromTime(long, WalletRescanReserver const&, bool) (wallet.cpp:1708)
 9==1787451==    by 0x5581DB: RescanWallet(CWallet&, WalletRescanReserver const&, long, bool) (rpcdump.cpp:85)
10==1787451==    by 0x57537E: importprivkey()::{lambda(RPCHelpMan const&, JSONRPCRequest const&)#1}::operator()(RPCHelpMan const&, JSONRPCRequest const&) const [clone .constprop.0] (rpcdump.cpp:188)
11==1787451==    by 0x575694: __invoke_impl<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&> (invoke.h:61)
12==1787451==    by 0x575694: __invoke_r<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&> (invoke.h:116)
13==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)
14==1787451==    by 0x2D4A9E: operator() (std_function.h:560)
15==1787451==    by 0x2D4A9E: HandleRequest (util.h:343)
16==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)
17==1787451==    by 0x43C473: operator() (std_function.h:560)
18==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)
19==1787451==    by 0x251DE4: operator() (std_function.h:560)
20==1787451==    by 0x251DE4: operator() (interfaces.cpp:382)
21==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)
22==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)
23==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)
24==1787451==    by 0x352364: operator() (std_function.h:560)
25==1787451==    by 0x352364: ExecuteCommand (server.cpp:466)
26==1787451==    by 0x352364: CRPCTable::execute(JSONRPCRequest const&) const (server.cpp:449)
27==1787451==    by 0x402AB9: HTTPReq_JSONRPC(util::Ref const&, HTTPRequest*) (httprpc.cpp:201)
28==1787451==    by 0x40ED8C: operator() (std_function.h:560)
29==1787451==    by 0x40ED8C: operator() (httpserver.cpp:49)
30==1787451==    by 0x40ED8C: WorkQueue<HTTPClosure>::Run() (httpserver.cpp:108)
31==1787451==    by 0x40A7C7: HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) (httpserver.cpp:336)
32==1787451== 
33{
34   <insert_a_suppression_name_here>
35   Memcheck:Cond
36   fun:_ZN7CWallet25ScanForWalletTransactionsERK7uint256iSt8optionalIiERK20WalletRescanReserverb
37   fun:_ZN7CWallet14RescanFromTimeElRK20WalletRescanReserverb
38   fun:_ZL12RescanWalletR7CWalletRK20WalletRescanReserverlb
39   fun:_ZZ13importprivkeyvENKUlRK10RPCHelpManRK14JSONRPCRequestE_clES1_S4_.constprop.0
40   fun:__invoke_impl<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&>
41   fun:__invoke_r<UniValue, importprivkey()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>&, const RPCHelpMan&, const JSONRPCRequest&>
42   fun:_ZNSt17_Function_handlerIF8UniValueRK10RPCHelpManRK14JSONRPCRequestEZ13importprivkeyvEUlS3_S6_E_E9_M_invokeERKSt9_Any_dataS3_S6_
43   fun:operator()
44   fun:HandleRequest
45   fun:_ZZN11CRPCCommandC4ENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES5_PF10RPCHelpManvESt6vectorIS5_SaIS5_EEENKUlRK14JSONRPCRequestR8UniValuebE_clESE_SG_b.isra.0
46   fun:operator()
47   fun:_ZZN6wallet12_GLOBAL__N_116WalletClientImpl12registerRpcsEvENKUlRK14JSONRPCRequestR8UniValuebE_clES4_S6_b.isra.0
48   fun:operator()
49   fun:operator()
50   fun:__invoke_impl<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool>
51   fun:__invoke_r<bool, node::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(const CRPCCommand&)::<lambda(const JSONRPCRequest&, UniValue&, bool)>&, const JSONRPCRequest&, UniValue&, bool>
52   fun:_ZNSt17_Function_handlerIFbRK14JSONRPCRequestR8UniValuebEZN4node12_GLOBAL__N_114RpcHandlerImplC4ERK11CRPCCommandEUlS2_S4_bE_E9_M_invokeERKSt9_Any_dataS2_S4_Ob
53   fun:operator()
54   fun:ExecuteCommand
55   fun:_ZNK9CRPCTable7executeERK14JSONRPCRequest
56   fun:_ZL15HTTPReq_JSONRPCRKN4util3RefEP11HTTPRequest
57   fun:operator()
58   fun:operator()
59   fun:_ZN9WorkQueueI11HTTPClosureE3RunEv
60   fun:_ZL16HTTPWorkQueueRunP9WorkQueueI11HTTPClosureEi
61}
62==1787451== 
63==1787451== Exit program on first error (--exit-on-first-error=yes) != 
64[node 0] Cleaning up leftover process

    compiler version

    019:29:11 -bash@camus ~/code/bitcoin/bitcoin/master 68c7acf6bb7$ gcc -v
1Using built-in specs.
2COLLECT_GCC=gcc
3COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/lto-wrapper
4Target: x86_64-pc-linux-gnu
5Configured 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
6Thread model: posix
7Supported LTO compression algorithms: zlib zstd
8gcc version 11.1.0 (GCC)
  33. apoelstra commented at 9:05 pm on August 3, 2021: contributor
    I think it is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635
  34. MarcoFalke commented at 5:43 am on August 4, 2021: member
    See also #18785, an unrelated but similar issue
  35. kittywhiskers referenced this in commit c1814a93b6 on May 7, 2022
  36. kittywhiskers referenced this in commit f9c3c387a1 on May 13, 2022
  37. kittywhiskers referenced this in commit 22c2c94f04 on May 17, 2022
  38. kittywhiskers referenced this in commit 4e438ebf87 on May 17, 2022
  39. kittywhiskers referenced this in commit 8fa55be72f on Jun 10, 2022
  40. kittywhiskers referenced this in commit e3217fcd56 on Jun 10, 2022
  41. kittywhiskers referenced this in commit c1c205bf88 on Jun 11, 2022
  42. kittywhiskers referenced this in commit bfcb64711c on Jun 11, 2022
  43. kittywhiskers referenced this in commit 93a20f0d52 on Jun 12, 2022
  44. kittywhiskers referenced this in commit f6a09a82f9 on Jun 12, 2022
  45. kittywhiskers referenced this in commit a52815ed1d on Jun 12, 2022
  46. kittywhiskers referenced this in commit bac909d9c0 on Jun 12, 2022
  47. kittywhiskers referenced this in commit aa8c497e5c on Jun 12, 2022
  48. kittywhiskers referenced this in commit a9d8c22840 on Jun 13, 2022
  49. kittywhiskers referenced this in commit bcac7cea26 on Jun 14, 2022
  50. kittywhiskers referenced this in commit a52dd8c2bd on Jun 18, 2022
  51. kittywhiskers referenced this in commit 0edb24b200 on Jun 23, 2022
  52. kittywhiskers referenced this in commit e235d834da on Jul 2, 2022
  53. PastaPastaPasta referenced this in commit 87b6d1c958 on Jul 3, 2022
  54. DrahtBot locked this on Aug 16, 2022


MarcoFalke laanwj hebasto jonatack jnewbery practicalswift DrahtBot promag apoelstra

Labels
Refactoring

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: 2024-12-25 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me