Skip to content

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Mar 5, 2021

As described in util/ref.h: "This implements a small subset of the functionality in C++17's std::any class, and can be dropped when the project updates to C++17". For accessing the contained object of a std::any instance, a helper template function AnyPtr is introduced (thanks to ryanofsky).

@fanquake
Copy link
Member

fanquake commented Mar 5, 2021

Concept ACK on removing no-longer needed abstraction. Currently failing to compile:

  CXX      wallet/libbitcoin_wallet_a-coincontrol.o
In file included from /usr/include/c++/9/bits/move.h:55,
                 from /usr/include/c++/9/bits/nested_exception.h:40,
                 from /usr/include/c++/9/exception:144,
                 from /usr/include/c++/9/new:40,
                 from /usr/include/c++/9/any:37,
                 from ./rpc/request.h:9,
                 from ./rpc/server.h:10,
                 from rpc/server.cpp:6:
/usr/include/c++/9/type_traits: In instantiation of ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’:
/usr/include/c++/9/type_traits:150:27:   required from ‘constexpr const bool std::__and_v<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’
/usr/include/c++/9/any:192:27:   required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> = <missing>]’
/usr/include/c++/9/type_traits:883:12:   required from ‘struct std::is_constructible<JSONRPCRequest, const JSONRPCRequest&>’
/usr/include/c++/9/type_traits:901:12:   required from ‘struct std::__is_copy_constructible_impl<JSONRPCRequest, true>’
/usr/include/c++/9/type_traits:907:12:   required from ‘struct std::is_copy_constructible<JSONRPCRequest>’
/usr/include/c++/9/type_traits:131:12:   required from ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >’
/usr/include/c++/9/any:181:58:   required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous>, typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous> = <missing>; typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> = <missing>]’
rpc/server.cpp:91:32:   required from here
/usr/include/c++/9/type_traits:136:12: error: incomplete type ‘std::is_copy_constructible<JSONRPCRequest>’ used in nested name specifier
  136 |     struct __and_<_B1, _B2, _B3, _Bn...>
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/9/type_traits: In instantiation of ‘constexpr const bool std::__and_v<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’:
/usr/include/c++/9/any:192:27:   required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> = <missing>]’
/usr/include/c++/9/type_traits:883:12:   required from ‘struct std::is_constructible<JSONRPCRequest, const JSONRPCRequest&>’
/usr/include/c++/9/type_traits:901:12:   required from ‘struct std::__is_copy_constructible_impl<JSONRPCRequest, true>’
/usr/include/c++/9/type_traits:907:12:   required from ‘struct std::is_copy_constructible<JSONRPCRequest>’
/usr/include/c++/9/type_traits:131:12:   required from ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >’
/usr/include/c++/9/any:181:58:   required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous>, typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous> = <missing>; typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> = <missing>]’
rpc/server.cpp:91:32:   required from here
/usr/include/c++/9/type_traits:150:27: error: ‘value’ is not a member of ‘std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’
  150 |     inline constexpr bool __and_v = __and_<_Bn...>::value;
      |                           ^~~~~~~
make[2]: *** [Makefile:8807: rpc/libbitcoin_server_a-server.o] Error 1

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@theStack
Copy link
Contributor Author

theStack commented Mar 5, 2021

Meh, there seems to be a problem with the (default) copy-constructor of class JSONRPCRequest that is invoked in CRPCTable::help:

... (..., const JSONRPCRequest& helpreq) {
    ...
    JSONRPCRequest jreq(helpreq);
    ...
}

No idea why this compiles locally with clang11 and obviously also for the CI jobs for MacOS and ARM.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this followup!

@theStack theStack force-pushed the 202012-replace-util_ref-by-std_any branch 2 times, most recently from 9309094 to 6595378 Compare March 5, 2021 14:54
@theStack
Copy link
Contributor Author

theStack commented Mar 5, 2021

Force-pushed with suggestions by @ryanofsky (#21366 (comment)): introduced a helper AnyPtr to access std::any contents and generally assigned pointers to std::any instances instead of references. Code looks much cleaner now.

@DrahtBot DrahtBot mentioned this pull request Mar 5, 2021
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review almost-ack 6595378. JSONRPCRequest std::any constructor isn't as safe as it could be because it can take precedence over the copy constructor

@theStack theStack force-pushed the 202012-replace-util_ref-by-std_any branch from 6595378 to 003e76c Compare March 6, 2021 19:15
@ryanofsky
Copy link
Contributor

Another batch of simplifications you could consider. There's no need really to require request objects to have contexts assigned.

diff

diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index 87c63593051..7d246325e44 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -131,7 +131,7 @@ static bool AppInit(int argc, char* argv[])
             // If locking the data directory failed, exit immediately
             return false;
         }
-        fRet = AppInitInterfaces(node) && AppInitMain(context, node);
+        fRet = AppInitInterfaces(node) && AppInitMain(node);
     }
     catch (const std::exception& e) {
         PrintExceptionContinue(&e, "AppInit()");
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index d33e671ee42..cd45eccc4ed 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -159,7 +159,8 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
         return false;
     }
 
-    JSONRPCRequest jreq(context);
+    JSONRPCRequest jreq;
+    jreq.context = context;
     jreq.peerAddr = req->GetPeer().ToString();
     if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
         LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", jreq.peerAddr);
@@ -294,7 +295,7 @@ bool StartHTTPRPC(const std::any& context)
     if (!InitRPCAuthentication())
         return false;
 
-    auto handle_rpc = [&context](HTTPRequest* req, const std::string&) { return HTTPReq_JSONRPC(context, req); };
+    auto handle_rpc = [context](HTTPRequest* req, const std::string&) { return HTTPReq_JSONRPC(context, req); };
     RegisterHTTPHandler("/", true, handle_rpc);
     if (g_wallet_init_interface.HasWalletSupport()) {
         RegisterHTTPHandler("/wallet/", false, handle_rpc);
diff --git a/src/init.cpp b/src/init.cpp
index efb92802126..c166af10f1f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -785,7 +785,7 @@ static bool InitSanityCheck()
     return true;
 }
 
-static bool AppInitServers(const std::any& context, NodeContext& node)
+static bool AppInitServers(NodeContext& node)
 {
     const ArgsManager& args = *Assert(node.args);
     RPCServer::OnStarted(&OnRPCStarted);
@@ -794,9 +794,9 @@ static bool AppInitServers(const std::any& context, NodeContext& node)
         return false;
     StartRPC();
     node.rpc_interruption_point = RpcInterruptionPoint;
-    if (!StartHTTPRPC(context))
+    if (!StartHTTPRPC(&node))
         return false;
-    if (args.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST(context);
+    if (args.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST(&node);
     StartHTTPServer();
     return true;
 }
@@ -1274,7 +1274,7 @@ bool AppInitInterfaces(NodeContext& node)
     return true;
 }
 
-bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
+bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 {
     const ArgsManager& args = *Assert(node.args);
     const CChainParams& chainparams = Params();
@@ -1379,7 +1379,7 @@ bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAn
      */
     if (args.GetBoolArg("-server", false)) {
         uiInterface.InitMessage_connect(SetRPCWarmupStatus);
-        if (!AppInitServers(context, node))
+        if (!AppInitServers(node))
             return InitError(_("Unable to start HTTP server. See debug log for details."));
     }
 
diff --git a/src/init.h b/src/init.h
index fe1c66202bf..d8fcb1e0ce3 100644
--- a/src/init.h
+++ b/src/init.h
@@ -59,7 +59,7 @@ bool AppInitInterfaces(NodeContext& node);
  * @note This should only be done after daemonization. Call Shutdown() if this function fails.
  * @pre Parameters should be parsed and config file should be read, AppInitLockDataDirectory should have been called.
  */
-bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr);
+bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr);
 
 /**
  * Register all arguments with the ArgsManager
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 8cf0dee5b4e..7d195688032 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -76,7 +76,7 @@ public:
     }
     bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
     {
-        return AppInitMain(m_context_ref, *m_context, tip_info);
+        return AppInitMain(*m_context, tip_info);
     }
     void appShutdown() override
     {
@@ -218,7 +218,8 @@ public:
     CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
     UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
     {
-        JSONRPCRequest req(m_context_ref);
+        JSONRPCRequest req;
+        req.context = m_context;
         req.params = params;
         req.strMethod = command;
         req.URI = uri;
@@ -287,14 +288,8 @@ public:
     void setContext(NodeContext* context) override
     {
         m_context = context;
-        if (context) {
-            m_context_ref = context;
-        } else {
-            m_context_ref.reset();
-        }
     }
     NodeContext* m_context{nullptr};
-    std::any m_context_ref;
 };
 
 bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active)
diff --git a/src/rest.cpp b/src/rest.cpp
index 452435f3e92..e2af5128121 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -317,7 +317,8 @@ static bool rest_chaininfo(const std::any& context, HTTPRequest* req, const std:
 
     switch (rf) {
     case RetFormat::JSON: {
-        JSONRPCRequest jsonRequest(context);
+        JSONRPCRequest jsonRequest;
+        jsonRequest.context = context;
         jsonRequest.params = UniValue(UniValue::VARR);
         UniValue chainInfoObject = getblockchaininfo().HandleRequest(jsonRequest);
         std::string strJSON = chainInfoObject.write() + "\n";
@@ -687,7 +688,7 @@ static const struct {
 void StartREST(const std::any& context)
 {
     for (const auto& up : uri_prefixes) {
-        auto handler = [&context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };
+        auto handler = [context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };
         RegisterHTTPHandler(up.prefix, false, handler);
     }
 }
diff --git a/src/rpc/request.h b/src/rpc/request.h
index 82d8fdc63ae..7a78d382cc9 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -35,18 +35,7 @@ public:
     std::string URI;
     std::string authUser;
     std::string peerAddr;
-    const std::any& context;
-
-    explicit JSONRPCRequest(const std::any& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
-
-    //! Initializes request information from another request object and the
-    //! given context. The implementation should be updated if any members are
-    //! added or removed above.
-    JSONRPCRequest(const JSONRPCRequest& other, const std::any& context)
-        : id(other.id), strMethod(other.strMethod), params(other.params), fHelp(other.fHelp), URI(other.URI),
-          authUser(other.authUser), peerAddr(other.peerAddr), context(context)
-    {
-    }
+    std::any context;
 
     void parse(const UniValue& valRequest);
 };
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 3d5216055ce..f16adf96d3e 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -88,7 +88,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
         vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
     sort(vCommands.begin(), vCommands.end());
 
-    JSONRPCRequest jreq(&helpreq);
+    JSONRPCRequest jreq(helpreq);
     jreq.fHelp = true;
     jreq.params = UniValue();
 
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
index 4ffc2478696..d2749c89599 100644
--- a/src/test/rpc_tests.cpp
+++ b/src/test/rpc_tests.cpp
@@ -33,8 +33,8 @@ UniValue RPCTestingSetup::CallRPC(std::string args)
     boost::split(vArgs, args, boost::is_any_of(" \t"));
     std::string strMethod = vArgs[0];
     vArgs.erase(vArgs.begin());
-    std::any context{&m_node};
-    JSONRPCRequest request(context);
+    JSONRPCRequest request;
+    request.context = &m_node;
     request.strMethod = strMethod;
     request.params = RPCConvertValues(strMethod, vArgs);
     request.fHelp = false;
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index c91ede94f95..46533d8704c 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -514,7 +514,9 @@ public:
     {
         for (const CRPCCommand& command : GetWalletRPCCommands()) {
             m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
-                return command.actor({request, &m_context}, result, last_handler);
+                JSONRPCRequest wrapped = request;
+                wrapped.context = &m_context;
+                return command.actor(wrapped, result, last_handler);
             }, command.argNames, command.unique_id);
             m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
         }
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index e48f7b4d4f5..a9c20921abd 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -213,8 +213,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
         key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1);
         key.pushKV("internal", UniValue(true));
         keys.push_back(key);
-        std::any context;
-        JSONRPCRequest request(context);
+        JSONRPCRequest request;
         request.params.setArray();
         request.params.push_back(keys);
 
@@ -265,8 +264,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
             AddWallet(wallet);
             wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
-        std::any context;
-        JSONRPCRequest request(context);
+        JSONRPCRequest request;
         request.params.setArray();
         request.params.push_back(backup_file);
 
@@ -281,8 +279,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
         LOCK(wallet->cs_wallet);
         wallet->SetupLegacyScriptPubKeyMan();
 
-        std::any context;
-        JSONRPCRequest request(context);
+        JSONRPCRequest request;
         request.params.setArray();
         request.params.push_back(backup_file);
         AddWallet(wallet);

@theStack theStack force-pushed the 202012-replace-util_ref-by-std_any branch from 003e76c to 6b6b899 Compare March 6, 2021 23:43
@theStack
Copy link
Contributor Author

theStack commented Mar 6, 2021

Force-pushed with feedback from @JeremyRubin (#21366 (comment)) and @ryanofsky (#21366 (comment)) taken into account: AnyPtr uses now the variant of std::any_cast that doesn't use exceptions. Also updated the PR description which still referred to the changeset without the AnyPtr helper and the exception variant of std::any_cast.

Another batch of simplifications you could consider. There's no need really to require request objects to have contexts assigned.

Good idea. I think those changes are out of scope for this PR, as the proposed simplifications are not connected to the util::Ref/std::any replacements. Happy to open a follow-up PR though or review if someone else does.

@practicalswift
Copy link
Contributor

Concept ACK

+89 −155 -- very nice! :)

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 6b6b899. Looks good! Only change since last review is simplifying AnyPtr function.

@theStack
Copy link
Contributor Author

Rebased on master.

@theStack theStack force-pushed the 202012-replace-util_ref-by-std_any branch from 3906949 to 916ab01 Compare March 29, 2021 21:35
@theStack
Copy link
Contributor Author

Rebased on master and took in all suggestions by @hebasto (#21366 (review)) -- thanks a lot for reviewing!

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 916ab01, with command

$ git range-diff master 39069499a61d7c2cab8594162c99bf00ea2ac88b 916ab0195d567fd0a9097045e73a6654c453adea

verified that since my previous review only rebased and implemented suggestions.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 916ab01. Changes since last review: rebase and replacing types with auto. I might have used const auto* and auto* instead of plain auto because I think the qualifiers are useful, but this is all good.

@laanwj laanwj merged commit 602b038 into bitcoin:master Mar 31, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 31, 2021
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 2, 2021
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:

https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking.  Suggested
change
bitcoin#21366 (comment)
eliminates these and would be a good followup for a future PR.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 2, 2021
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 3, 2021
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:

https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking.  Suggested
change
bitcoin#21366 (comment)
eliminates these and would be a good followup for a future PR.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 3, 2021
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Apr 7, 2021
937fd4a Fix wrong wallet RPC context set after #21366 (Russell Yanofsky)

Pull request description:

  This bug doesn't have any effects currently because it only affects
  external signer RPCs which aren't currently using the wallet context,
  but it does cause an appveyor failure in a upcoming PR:

  https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

  This bug is subtle and could have been avoided if JSONRPCRequest didn't
  have constructors that were so loose with type checking.  Suggested
  change
  bitcoin/bitcoin#21366 (comment)
  eliminates these and would be a good followup for a future PR.

  This PR just implements the simplest possible fix.

ACKs for top commit:
  theStack:
    Code-review ACK 937fd4a
  meshcollider:
    Code review ACK 937fd4a

Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 7, 2021
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 7, 2021
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 7, 2021
…1366

937fd4a Fix wrong wallet RPC context set after bitcoin#21366 (Russell Yanofsky)

Pull request description:

  This bug doesn't have any effects currently because it only affects
  external signer RPCs which aren't currently using the wallet context,
  but it does cause an appveyor failure in a upcoming PR:

  https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

  This bug is subtle and could have been avoided if JSONRPCRequest didn't
  have constructors that were so loose with type checking.  Suggested
  change
  bitcoin#21366 (comment)
  eliminates these and would be a good followup for a future PR.

  This PR just implements the simplest possible fix.

ACKs for top commit:
  theStack:
    Code-review ACK 937fd4a
  meshcollider:
    Code review ACK 937fd4a

Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Apr 8, 2021
9044522 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky)

Pull request description:

  This just makes an additional simplification after #21366 replaced
  util::Ref with std::any. It was originally suggested
  bitcoin/bitcoin#21366 (comment) but
  delayed for a followup. It would have prevented usage bug
  bitcoin/bitcoin#21572.

ACKs for top commit:
  promag:
    ACK 9044522, fixed conflict in src/wallet/interfaces.cpp.

Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 8, 2021
…1366

9044522 Drop JSONRPCRequest constructors after bitcoin#21366 (Russell Yanofsky)

Pull request description:

  This just makes an additional simplification after bitcoin#21366 replaced
  util::Ref with std::any. It was originally suggested
  bitcoin#21366 (comment) but
  delayed for a followup. It would have prevented usage bug
  bitcoin#21572.

ACKs for top commit:
  promag:
    ACK 9044522, fixed conflict in src/wallet/interfaces.cpp.

Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 8, 2021
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:

https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking.  Suggested
change
bitcoin#21366 (comment)
eliminates these and would be a good followup for a future PR.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 8, 2021
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
hebasto added a commit to hebasto/bitcoin-maintainer-tools that referenced this pull request May 5, 2021
This change is required since bitcoin/bitcoin#21366 was merged.
@bitcoin bitcoin deleted a comment May 16, 2021
@bitcoin bitcoin locked and limited conversation to collaborators May 16, 2021
@theStack theStack deleted the 202012-replace-util_ref-by-std_any branch July 31, 2021 20:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants