refactor: replace util::Ref with std::any (C++17) #21366

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

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

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

• #20459 (rpc: Fail to return undocumented return values by MarcoFalke)
• #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)

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.

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.

Thanks for this followup!

Force-pushed with suggestions by @ryanofsky (https://github.com/bitcoin/bitcoin/pull/21366#discussion_r588287531): 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.

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

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

<details><summary>diff</summary> <p>

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](/bitcoin-bitcoin/contributor/note/) This should only be done after daemonization. Call Shutdown() if this function fails.
  * [@pre](/bitcoin-bitcoin/contributor/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);

</p> </details>

Force-pushed with feedback from @JeremyRubin (https://github.com/bitcoin/bitcoin/pull/21366#discussion_r588921151) and @ryanofsky (https://github.com/bitcoin/bitcoin/pull/21366#discussion_r588923880) 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.

Concept ACK

+89 −155 -- very nice! :)

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

Rebased on master.

Rebased on master again.

Concept ACK.

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

Rebased on master and took in all suggestions by @hebasto (https://github.com/bitcoin/bitcoin/pull/21366#pullrequestreview-622716405) -- thanks a lot for reviewing!

re-ACK 916ab0195d567fd0a9097045e73a6654c453adea, with command

$ git range-diff master 39069499a61d7c2cab8594162c99bf00ea2ac88b 916ab0195d567fd0a9097045e73a6654c453adea

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

Code review ACK 916ab0195d567fd0a9097045e73a6654c453adea. 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.