Wallet/RPC: Add listsincetx method with a stateless (server-side) long polling option #13262

pull jonasschnelli wants to merge 4 commits into bitcoin:master from jonasschnelli:2018/05/listsincetx changing 7 files +278 −0
  1. jonasschnelli commented at 11:56 am on May 17, 2018: contributor

    This adds a new wallet RPC call listsincetx that allows to list follow-up transactions (in order / wtxOrdered) from a specified transaction.

    If the latest transaction was used as base-point for finding follow-up transactions and the long-polling timeout if set greater then zero, it will wait until new transactions has been found or the timeout has been expired (http push channel). Used together with a https proxy, one can build a very robust wallet transaction push channel (that can bypass NATs, proxys, etc.)

    This does allow to build a wallet-notify push channel without keeping client-states on the server side. It does also allow clients to effectively sync the transaction list.

    A client can simply loop over listsincetx(<newest_known_txid>, 30/*timeout*/) and will immediately get new txns once they arrive.

    Mitigates #13237 Related #7949

    This (or a similar) approach was once discussed during an IRC meeting (can’t find it anymore in the logs).

    • Needs release notes
    • Needs wallet notify example python script
  2. jonasschnelli added the label Wallet on May 17, 2018
  3. jonasschnelli added the label RPC/REST/ZMQ on May 17, 2018
  4. jonasschnelli force-pushed on May 17, 2018
  5. in src/wallet/wallet.cpp:996 in 3899ae1aee outdated 
    989@@ -990,6 +990,21 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
990         t.detach(); // thread runs free
991     }
992 
993+    {
994+        // update the latest transaction id in case it has changed
995+        std::lock_guard<std::mutex> lock(m_cs_txadd);
996+        if(!wtxOrdered.empty()) {
    promag commented at 1:08 pm on May 17, 2018:
    nit, space after if.
  6. in src/wallet/wallet.cpp:998 in 3899ae1aee outdated 
    989@@ -990,6 +990,21 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
990         t.detach(); // thread runs free
991     }
992 
993+    {
994+        // update the latest transaction id in case it has changed
995+        std::lock_guard<std::mutex> lock(m_cs_txadd);
996+        if(!wtxOrdered.empty()) {
997+            CWalletTx *pwtx = (--wtxOrdered.end())->second.first;
998+            if (pwtx) {
    promag commented at 1:09 pm on May 17, 2018:
    Can this be nullptr? Could assert(pwtx) instead?
  7. in src/wallet/wallet.h:1187 in 3899ae1aee outdated 
    1179@@ -1180,6 +1180,11 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1180 
1181     /** Whether a given output is spendable by this wallet */
1182     bool OutputEligibleForSpending(const COutput& output, const CoinEligibilityFilter& eligibility_filter) const;
1183+
1184+    /** mutex, container and condition variable for updating the latest transaction id */
1185+    std::mutex m_cs_txadd;
1186+    std::condition_variable m_cond_txadd;
1187+    uint256 m_latest_wtxid;
    promag commented at 1:18 pm on May 17, 2018:

    Could keep wtxOrdered back iterator instead:

    0TxItems::const_iterator m_latest_wtx = wtxOrdered.rbegin();

    Then above could be:

    0if (m_latest_wtxid != wtxOrdered.rbegin()) {
1    ...
2}
    jonasschnelli commented at 4:00 pm on May 17, 2018:
    I prefer to keep a non references object in this case.
  8. in src/wallet/wallet.cpp:997 in 3899ae1aee outdated 
    989@@ -990,6 +990,21 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
990         t.detach(); // thread runs free
991     }
992 
993+    {
994+        // update the latest transaction id in case it has changed
995+        std::lock_guard<std::mutex> lock(m_cs_txadd);
996+        if(!wtxOrdered.empty()) {
997+            CWalletTx *pwtx = (--wtxOrdered.end())->second.first;
    promag commented at 1:19 pm on May 17, 2018: 
    0... = wtxOrdered.rbegin()->second.first;
  9. in src/wallet/wallet.cpp:1029 in 3899ae1aee outdated 
    1023@@ -1009,6 +1024,10 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
1024             }
1025         }
1026     }
1027+    if (!wtxOrdered.empty()) {
1028+        std::lock_guard<std::mutex> lock(m_cs_txadd);
1029+        m_latest_wtxid = wtxOrdered.rbegin()->second.first->GetHash();
1030+    }
    promag commented at 1:20 pm on May 17, 2018:
    Missing notify on m_cond_txadd?
  10. in src/wallet/rpcwallet.cpp:2129 in 016d27a626 outdated 
    2124+        return NullUniValue;
2125+    }
2126+
2127+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2128+        throw std::runtime_error(
2129+            "listsincetx ( \"txid\" poll )\n"
    promag commented at 1:22 pm on May 17, 2018:
    txid is required, should be out of ( ).
  11. in src/wallet/rpcwallet.cpp:2150 in 016d27a626 outdated 
    2145+    // we assume that pwallet gets never deconstructed at this point
2146+    uint256 hash;
2147+    hash.SetHex(request.params[0].get_str());
2148+    auto it_find = pwallet->mapWallet.find(hash);
2149+    if (it_find == pwallet->mapWallet.end()) {
2150+        throw JSONRPCError(RPC_INVALID_PARAMETER, "transaction not found");
    promag commented at 1:24 pm on May 17, 2018:

    Should be

    0throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid or non-wallet transaction id");

    which is the error used in other calls.

  12. in src/wallet/rpcwallet.cpp:2180 in 016d27a626 outdated 
    2175+    // find transaction and eventually populate the array with follow-up transactions
2176+    addTransactionsSince(it_find->first);
2177+
2178+    // in case nothing has been found (means newest transaction has been requested) and requested txid is valid, longpoll if requested
2179+    if (transactions.empty() && request.params[1].isNum() && request.params[1].get_int() > 0)
2180+    {
    promag commented at 1:24 pm on May 17, 2018:
    nit move up
    Empact commented at 4:27 pm on May 17, 2018:
    clang-format would fix a few other minor things as well
  13. in src/wallet/rpcwallet.cpp:2134 in 016d27a626 outdated 
    2129+            "listsincetx ( \"txid\" poll )\n"
2130+            "\nGet all transactions since a specific transaction [txid].\n"
2131+            "This call can be used as longpoll notification channel if follow-up transactions from the latest transaction-id are requested and [polltime] is greater then zero.\n"
2132+            "\nArguments:\n"
2133+            "1. \"txid\"               (string, required) The txid to list transactions since\n"
2134+            "1. \"polltime\"           (numeric, optional) If greater then zero, wait with a timeout of <polltime> seconds for new transactions. It will response immediately when new transactions arrive (default is 0)\n"
    promag commented at 1:24 pm on May 17, 2018:
    arg name should be poll?
    jonasschnelli commented at 4:41 pm on May 17, 2018:
    Fixed the above; polltime is correct
  14. in src/wallet/rpcwallet.cpp:2136 in 016d27a626 outdated 
    2131+            "This call can be used as longpoll notification channel if follow-up transactions from the latest transaction-id are requested and [polltime] is greater then zero.\n"
2132+            "\nArguments:\n"
2133+            "1. \"txid\"               (string, required) The txid to list transactions since\n"
2134+            "1. \"polltime\"           (numeric, optional) If greater then zero, wait with a timeout of <polltime> seconds for new transactions. It will response immediately when new transactions arrive (default is 0)\n"
2135+            "\nExamples:\n"
2136+            + HelpExampleCli("listsincetx", "")
    promag commented at 1:25 pm on May 17, 2018:
    Remove this invalid usage?
  15. promag commented at 1:29 pm on May 17, 2018: member

    Concept ACK.

    I’m not aware of the wtxOrdered details, but is the key always increasing? Also, it’s a multimap so the same key can have multiple transactions, isn’t that a problem?

    Some comments though.

  16. jonasschnelli renamed this:
    Wallet/RPC: Add listsincetx with a stateless (server-side) long polling option
    Wallet/RPC: Add listsincetx method with a stateless (server-side) long polling option
    on May 17, 2018
  17. jonasschnelli force-pushed on May 17, 2018
  18. Keep track of the latest wtxid c36e0083c3
  19. jonasschnelli force-pushed on May 17, 2018
  20. jonasschnelli commented at 4:42 pm on May 17, 2018: contributor
    • Fixed points found by @promag, ran clang-format
    • Added an example python script that shows the push channel capabilities
  21. in src/wallet/rpcwallet.cpp:4268 in eff17065b1 outdated 
    4264@@ -4194,6 +4265,8 @@ static const CRPCCommand commands[] =
4265     { "wallet",             "listlockunspent",                  &listlockunspent,               {} },
4266     { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly","address_filter"} },
4267     { "wallet",             "listsinceblock",                   &listsinceblock,                {"blockhash","target_confirmations","include_watchonly","include_removed"} },
4268+    { "wallet",             "listsincetx",                      &listsincetx,                   {"txid", "poll"} },
    promag commented at 4:44 pm on May 17, 2018:
    polltime.
  22. in src/rpc/client.cpp:157 in eff17065b1 outdated 
    153@@ -154,6 +154,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
154     { "echojson", 9, "arg9" },
155     { "rescanblockchain", 0, "start_height"},
156     { "rescanblockchain", 1, "stop_height"},
157+    { "listsincetx", 1, "poll"},
    promag commented at 4:44 pm on May 17, 2018:
    polltime.
  23. in contrib/push/example_walletnotify.py:115 in eff17065b1 outdated 
    110+    latest_wtx = rpc.execute(rpc.build_request(0, "listtransactions", ["*", 1]))['result'][-1]['txid']
111+    while True:
112+        res = rpc.execute(rpc.build_request(0, "listsincetx", [latest_wtx, 100]))
113+        for tx in res['result']:
114+            print("New txid"+tx['txid']+" with amount "+str(tx['amount']))
115+        latest_wtx = res['result'][-1]['txid'] #update latest wtx
    promag commented at 4:44 pm on May 17, 2018:
    nit, add newline.
  24. promag commented at 4:45 pm on May 17, 2018: member
    Will play around, just some nits that could be fixed meanwhile.
  25. jonasschnelli force-pushed on May 17, 2018
  26. jonasschnelli force-pushed on May 17, 2018
  27. promag commented at 4:53 pm on May 17, 2018: member
    You could note that this is not the same as -walletnotify because that also notifies when a wallet transaction state changes between mempool <-> block.
  28. jonasschnelli commented at 5:00 pm on May 17, 2018: contributor

    You could note that this is not the same as -walletnotify because that also notifies when a wallet transaction state changes between mempool <-> block.

    Good point. Conceptually, I think that confirmation change should be detected with another push function (non wallet general “tip has changes” like function).

  29. in contrib/push/example_walletnotify.py:3 in 2f505ac2d3 outdated 
    0@@ -0,0 +1,114 @@
1+#!/usr/bin/env python3
2+#
3+# Examplecode for wallet push notifications (walletnotify)
    promag commented at 5:18 pm on May 17, 2018:
    Nit, space example code.
  30. in contrib/push/example_walletnotify.py:4 in 2f505ac2d3 outdated 
    0@@ -0,0 +1,114 @@
1+#!/usr/bin/env python3
2+#
3+# Examplecode for wallet push notifications (walletnotify)
4+# ./walletnotify.py -network=regtest -datadir=<path>/regtest/
    promag commented at 5:19 pm on May 17, 2018:
    Fix usage.
  31. in contrib/push/example_walletnotify.py:18 in 2f505ac2d3 outdated 
    11+from __future__ import print_function
12+try: # Python 3
13+    import http.client as httplib
14+except ImportError: # Python 2
15+    import httplib
16+import json
    promag commented at 5:19 pm on May 17, 2018:
    nit, sort these.
    jonasschnelli commented at 6:43 pm on May 17, 2018:
    Fixed.
  32. in src/wallet/rpcwallet.cpp:2134 in 2f505ac2d3 outdated 
    2129+            "listsincetx \"txid\" (polltime)\n"
2130+            "\nGet all transactions since a specific transaction [txid].\n"
2131+            "This call can be used as longpoll notification channel if follow-up transactions from the latest transaction-id are requested and [polltime] is greater then zero.\n"
2132+            "\nArguments:\n"
2133+            "1. \"txid\"               (string, required) The txid to list transactions since\n"
2134+            "1. \"polltime\"           (numeric, optional) If greater then zero, wait with a timeout of <polltime> seconds for new transactions. It will response immediately when new transactions arrive (default is 0)\n"
    promag commented at 5:21 pm on May 17, 2018:
    greater than zero
    jonasschnelli commented at 6:29 pm on May 17, 2018:
    I don’t understand that comment.
    promag commented at 6:38 pm on May 17, 2018:
    s/then/than: If greater than zero, wait with ...
  33. in src/wallet/rpcwallet.cpp:2156 in 2f505ac2d3 outdated 
    2151+
2152+    UniValue transactions(UniValue::VARR);
2153+
2154+    // construct a function to populate the output array
2155+    auto addTransactionsSince = [&transactions, &pwallet](uint256 wtxid_since) {
2156+        LOCK2(cs_main, pwallet->cs_wallet); // we only lock at this point to allow non blocking long polling
    promag commented at 5:23 pm on May 17, 2018:
    Why lock cs_main?
    jonasschnelli commented at 6:32 pm on May 17, 2018:
    IMO cs_main should always be locked before cs_wallet, especially with the usage of ListTransactions() and its subclass.
  34. in src/wallet/rpcwallet.cpp:2129 in 2f505ac2d3 outdated 
    2124+        return NullUniValue;
2125+    }
2126+
2127+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2128+        throw std::runtime_error(
2129+            "listsincetx \"txid\" (polltime)\n"
    promag commented at 5:24 pm on May 17, 2018:
    Could have a limit argument? Otherwise it would dump the whole wallet?
    jonasschnelli commented at 6:30 pm on May 17, 2018:
    I thought of it,… but would like to keep it simple for a first implementation (limit could follow next)
  35. in src/wallet/rpcwallet.cpp:2176 in 2f505ac2d3 outdated 
    2171+
2172+    // find transaction and eventually populate the array with follow-up transactions
2173+    addTransactionsSince(it_find->first);
2174+
2175+    // in case nothing has been found (means newest transaction has been requested) and requested txid is valid, longpoll if requested
2176+    if (transactions.empty() && request.params[1].isNum() && request.params[1].get_int() > 0) {
    promag commented at 5:27 pm on May 17, 2018:

    Should not use request.params[1].isNum() otherwise the caller can pass a string (for example) and it won’t raise an error. I suggest to change to:

    0if (transactions.empty() && !request.params[1].isNull() && request.params[1].get_int() > 0)
  36. promag commented at 5:32 pm on May 17, 2018: member
    Another round.
  37. jonasschnelli force-pushed on May 17, 2018
  38. RPC: add listsincetx, a way to longpoll for new wallet transactions 7fa78fa77c
  39. QA: add ListSinceTxTest test a6535bb3e5
  40. Contrib: add walletnotify example code 2dadbc883e
  41. jonasschnelli force-pushed on May 17, 2018
  42. DrahtBot added the label Needs rebase on Jul 20, 2018
  43. DrahtBot commented at 10:39 pm on July 20, 2018: member
  44. DrahtBot commented at 1:41 pm on October 28, 2018: member
  45. DrahtBot added the label Up for grabs on Oct 28, 2018
  46. DrahtBot closed this on Oct 28, 2018
  47. laanwj removed the label Needs rebase on Oct 24, 2019
  48. MarcoFalke locked this on Dec 16, 2021


jonasschnelli promag Empact DrahtBot

Labels
Wallet RPC/REST/ZMQ Up for grabs

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