Merge bitcoin#32862: rpc: use CScheduler for relocking wallet and remove RPCTimer

fcfd3db remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a17657 use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)

Pull request description:

  This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.

  This is an alternative approach to bitcoin#32796, described in bitcoin#32796 (comment)

ACKs for top commit:
  fjahr:
    Code Review ACK fcfd3db
  achow101:
    ACK fcfd3db
  furszy:
    ACK fcfd3db

Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14

Author: achow101

src/httprpc.cpp

@@ -32,45 +32,6 @@ using util::TrimStringView;
 /** WWW-Authenticate to present with 401 Unauthorized response */
 static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
 
-/** Simple one-shot callback timer to be used by the RPC mechanism to e.g.
- * re-lock the wallet.
- */
-class HTTPRPCTimer : public RPCTimerBase
-{
-public:
-    HTTPRPCTimer(struct event_base* eventBase, std::function<void()>& func, int64_t millis) :
-        ev(eventBase, false, func)
-    {
-        struct timeval tv;
-        tv.tv_sec = millis/1000;
-        tv.tv_usec = (millis%1000)*1000;
-        ev.trigger(&tv);
-    }
-private:
-    HTTPEvent ev;
-};
-
-class HTTPRPCTimerInterface : public RPCTimerInterface
-{
-public:
-    explicit HTTPRPCTimerInterface(struct event_base* _base) : base(_base)
-    {
-    }
-    const char* Name() override
-    {
-        return "HTTP";
-    }
-    RPCTimerBase* NewTimer(std::function<void()>& func, int64_t millis) override
-    {
-        return new HTTPRPCTimer(base, func, millis);
-    }
-private:
-    struct event_base* base;
-};
-
-
-/* Stored RPC timer interface (for unregistration) */
-static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
 /* List of -rpcauth values */
 static std::vector<std::vector<std::string>> g_rpcauth;
 /* RPC Auth Whitelist */
@@ -380,8 +341,6 @@ bool StartHTTPRPC(const std::any& context)
     }
     struct event_base* eventBase = EventBase();
     assert(eventBase);
-    httpRPCTimerInterface = std::make_unique<HTTPRPCTimerInterface>(eventBase);
-    RPCSetTimerInterface(httpRPCTimerInterface.get());
     return true;
 }
 
@@ -397,8 +356,4 @@ void StopHTTPRPC()
     if (g_wallet_init_interface.HasWalletSupport()) {
         UnregisterHTTPHandler("/wallet/", false);
     }
-    if (httpRPCTimerInterface) {
-        RPCUnsetTimerInterface(httpRPCTimerInterface.get());
-        httpRPCTimerInterface.reset();
-    }
 }

src/interfaces/chain.h

@@ -352,9 +352,6 @@ class Chain
     //! Check if deprecated RPC is enabled.
     virtual bool rpcEnableDeprecated(const std::string& method) = 0;
 
-    //! Run function after given number of seconds. Cancel any previous calls with same name.
-    virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;
-
     //! Get settings value.
     virtual common::SettingsValue getSetting(const std::string& arg) = 0;
 

src/interfaces/node.h

@@ -27,7 +27,6 @@ class BanMan;
 class CFeeRate;
 class CNodeStats;
 class Coin;
-class RPCTimerInterface;
 class UniValue;
 class Proxy;
 enum class SynchronizationState;
@@ -205,12 +204,6 @@ class Node
     //! List rpc commands.
     virtual std::vector<std::string> listRpcCommands() = 0;
 
-    //! Set RPC timer interface if unset.
-    virtual void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) = 0;
-
-    //! Unset RPC timer interface.
-    virtual void rpcUnsetTimerInterface(RPCTimerInterface* iface) = 0;
-
     //! Get unspent output associated with a transaction.
     virtual std::optional<Coin> getUnspentOutput(const COutPoint& output) = 0;
 

src/node/interfaces.cpp

@@ -356,8 +356,6 @@ class NodeImpl : public Node
         return ::tableRPC.execute(req);
     }
     std::vector<std::string> listRpcCommands() override { return ::tableRPC.listCommands(); }
-    void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) override { RPCSetTimerInterfaceIfUnset(iface); }
-    void rpcUnsetTimerInterface(RPCTimerInterface* iface) override { RPCUnsetTimerInterface(iface); }
     std::optional<Coin> getUnspentOutput(const COutPoint& output) override
     {
         LOCK(::cs_main);
@@ -804,10 +802,6 @@ class ChainImpl : public Chain
         return std::make_unique<RpcHandlerImpl>(command);
     }
     bool rpcEnableDeprecated(const std::string& method) override { return IsDeprecatedRPCEnabled(method); }
-    void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) override
-    {
-        RPCRunLater(name, std::move(fn), seconds);
-    }
     common::SettingsValue getSetting(const std::string& name) override
     {
         return args().GetSetting(name);

src/qt/rpcconsole.cpp

@@ -98,37 +98,6 @@ public Q_SLOTS:
     interfaces::Node& m_node;
 };
 
-/** Class for handling RPC timers
- * (used for e.g. re-locking the wallet after a timeout)
- */
-class QtRPCTimerBase: public QObject, public RPCTimerBase
-{
-    Q_OBJECT
-public:
-    QtRPCTimerBase(std::function<void()>& _func, int64_t millis):
-        func(_func)
-    {
-        timer.setSingleShot(true);
-        connect(&timer, &QTimer::timeout, [this]{ func(); });
-        timer.start(millis);
-    }
-    ~QtRPCTimerBase() = default;
-private:
-    QTimer timer;
-    std::function<void()> func;
-};
-
-class QtRPCTimerInterface: public RPCTimerInterface
-{
-public:
-    ~QtRPCTimerInterface() = default;
-    const char *Name() override { return "Qt"; }
-    RPCTimerBase* NewTimer(std::function<void()>& func, int64_t millis) override
-    {
-        return new QtRPCTimerBase(func, millis);
-    }
-};
-
 class PeerIdViewDelegate : public QStyledItemDelegate
 {
     Q_OBJECT
@@ -567,12 +536,6 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
     ui->WalletSelector->setVisible(false);
     ui->WalletSelectorLabel->setVisible(false);
 
-    // Register RPC timer interface
-    rpcTimerInterface = new QtRPCTimerInterface();
-    // avoid accidentally overwriting an existing, non QTThread
-    // based timer interface
-    m_node.rpcSetTimerInterfaceIfUnset(rpcTimerInterface);
-
     setTrafficGraphRange(INITIAL_TRAFFIC_GRAPH_MINS);
     updateDetailWidget();
 
@@ -602,8 +565,6 @@ RPCConsole::~RPCConsole()
     settings.setValue("PeersTabPeerHeaderState", m_peer_widget_header_state);
     settings.setValue("PeersTabBanlistHeaderState", m_banlist_widget_header_state);
 
-    m_node.rpcUnsetTimerInterface(rpcTimerInterface);
-    delete rpcTimerInterface;
     delete ui;
 }
 

src/qt/rpcconsole.h

@@ -20,7 +20,6 @@
 
 class PlatformStyle;
 class RPCExecutor;
-class RPCTimerInterface;
 class WalletModel;
 
 namespace interfaces {
@@ -166,7 +165,6 @@ public Q_SLOTS:
     QString cmdBeforeBrowsing;
     QList<NodeId> cachedNodeids;
     const PlatformStyle* const platformStyle;
-    RPCTimerInterface *rpcTimerInterface = nullptr;
     QMenu *peersTableContextMenu = nullptr;
     QMenu *banTableContextMenu = nullptr;
     int consoleFontSize = 0;

src/rpc/server.cpp

@@ -34,11 +34,6 @@ static GlobalMutex g_rpc_warmup_mutex;
 static std::atomic<bool> g_rpc_running{false};
 static bool fRPCInWarmup GUARDED_BY(g_rpc_warmup_mutex) = true;
 static std::string rpcWarmupStatus GUARDED_BY(g_rpc_warmup_mutex) = "RPC server started";
-/* Timer-creating functions */
-static RPCTimerInterface* timerInterface = nullptr;
-/* Map of name to timer. */
-static GlobalMutex g_deadline_timers_mutex;
-static std::map<std::string, std::unique_ptr<RPCTimerBase> > deadlineTimers GUARDED_BY(g_deadline_timers_mutex);
 static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler);
 
 struct RPCCommandExecutionInfo
@@ -301,7 +296,6 @@ void StopRPC()
     assert(!g_rpc_running);
     std::call_once(g_rpc_stop_flag, [&]() {
         LogDebug(BCLog::RPC, "Stopping RPC\n");
-        WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear());
         DeleteAuthCookie();
         LogDebug(BCLog::RPC, "RPC stopped.\n");
     });
@@ -543,31 +537,4 @@ UniValue CRPCTable::dumpArgMap(const JSONRPCRequest& args_request) const
     return ret;
 }
 
-void RPCSetTimerInterfaceIfUnset(RPCTimerInterface *iface)
-{
-    if (!timerInterface)
-        timerInterface = iface;
-}
-
-void RPCSetTimerInterface(RPCTimerInterface *iface)
-{
-    timerInterface = iface;
-}
-
-void RPCUnsetTimerInterface(RPCTimerInterface *iface)
-{
-    if (timerInterface == iface)
-        timerInterface = nullptr;
-}
-
-void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds)
-{
-    if (!timerInterface)
-        throw JSONRPCError(RPC_INTERNAL_ERROR, "No timer handler registered for RPC");
-    LOCK(g_deadline_timers_mutex);
-    deadlineTimers.erase(name);
-    LogDebug(BCLog::RPC, "queue run of timer %s in %i seconds (using %s)\n", name, nSeconds, timerInterface->Name());
-    deadlineTimers.emplace(name, std::unique_ptr<RPCTimerBase>(timerInterface->NewTimer(func, nSeconds*1000)));
-}
-
 CRPCTable tableRPC;

src/rpc/server.h

@@ -35,47 +35,6 @@ void SetRPCWarmupFinished();
 /* returns the current warmup state.  */
 bool RPCIsInWarmup(std::string *outStatus);
 
-/** Opaque base class for timers returned by NewTimerFunc.
- * This provides no methods at the moment, but makes sure that delete
- * cleans up the whole state.
- */
-class RPCTimerBase
-{
-public:
-    virtual ~RPCTimerBase() = default;
-};
-
-/**
- * RPC timer "driver".
- */
-class RPCTimerInterface
-{
-public:
-    virtual ~RPCTimerInterface() = default;
-    /** Implementation name */
-    virtual const char *Name() = 0;
-    /** Factory function for timers.
-     * RPC will call the function to create a timer that will call func in *millis* milliseconds.
-     * @note As the RPC mechanism is backend-neutral, it can use different implementations of timers.
-     * This is needed to cope with the case in which there is no HTTP server, but
-     * only GUI RPC console, and to break the dependency of pcserver on httprpc.
-     */
-    virtual RPCTimerBase* NewTimer(std::function<void()>& func, int64_t millis) = 0;
-};
-
-/** Set the factory function for timers */
-void RPCSetTimerInterface(RPCTimerInterface *iface);
-/** Set the factory function for timer, but only, if unset */
-void RPCSetTimerInterfaceIfUnset(RPCTimerInterface *iface);
-/** Unset factory function for timers */
-void RPCUnsetTimerInterface(RPCTimerInterface *iface);
-
-/**
- * Run func nSeconds from now.
- * Overrides previous timer <name> (if any).
- */
-void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds);
-
 typedef RPCHelpMan (*RpcMethodFnType)();
 
 class CRPCCommand

src/wallet/rpc/encrypt.cpp

@@ -3,6 +3,8 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <rpc/util.h>
+#include <scheduler.h>
+#include <wallet/context.h>
 #include <wallet/rpc/util.h>
 #include <wallet/wallet.h>
 
@@ -88,24 +90,24 @@ RPCHelpMan walletpassphrase()
         relock_time = pwallet->nRelockTime;
     }
 
-    // rpcRunLater must be called without cs_wallet held otherwise a deadlock
-    // can occur. The deadlock would happen when RPCRunLater removes the
-    // previous timer (and waits for the callback to finish if already running)
-    // and the callback locks cs_wallet.
-    AssertLockNotHeld(wallet->cs_wallet);
+    // Get wallet scheduler to queue up the relock callback in the future.
+    // Scheduled events don't get destructed until they are executed,
+    // and they are executed in series in a single scheduler thread so
+    // no cs_wallet lock is needed.
+    WalletContext& context = EnsureWalletContext(request.context);
     // Keep a weak pointer to the wallet so that it is possible to unload the
     // wallet before the following callback is called. If a valid shared pointer
     // is acquired in the callback then the wallet is still loaded.
     std::weak_ptr<CWallet> weak_wallet = wallet;
-    pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet, relock_time] {
+    context.scheduler->scheduleFromNow([weak_wallet, relock_time] {
         if (auto shared_wallet = weak_wallet.lock()) {
             LOCK2(shared_wallet->m_relock_mutex, shared_wallet->cs_wallet);
-            // Skip if this is not the most recent rpcRunLater callback.
+            // Skip if this is not the most recent relock callback.
             if (shared_wallet->nRelockTime != relock_time) return;
             shared_wallet->Lock();
             shared_wallet->nRelockTime = 0;
         }
-    }, nSleepTime);
+    }, std::chrono::seconds(nSleepTime));
 
     return UniValue::VNULL;
 },

test/functional/wallet_keypool.py

@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the wallet keypool and interaction with wallet encryption/locking."""
 
-import time
 from decimal import Decimal
 
 from test_framework.test_framework import BitcoinTestFramework
@@ -127,8 +126,10 @@ def run_test(self):
         nodes[0].keypoolrefill(3)
 
         # test walletpassphrase timeout
-        time.sleep(1.1)
-        assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
+        # CScheduler relies on condition_variable::wait_until() which does not
+        # guarantee accurate timing. We'll wait up to 5 seconds to execute a 1
+        # second scheduled event.
+        nodes[0].wait_until(lambda: nodes[0].getwalletinfo()["unlocked_until"] == 0, timeout=5)
 
         # drain the keypool
         for _ in range(3):