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:

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

    Then above could be:

    if (m_latest_wtxid != wtxOrdered.rbegin()) {
    ...
}
    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: 
    ... = 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

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

    if (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. jonasschnelli cross-referenced this on May 23, 2018 from issue Possibly missing updates from Bitcoind backend by tyzbit
  43. jonasschnelli cross-referenced this on May 24, 2018 from issue configurable ZMQ message limit by Alex-CodeLab
  44. jonasschnelli cross-referenced this on May 30, 2018 from issue wallet: Replace %w by wallet name in -walletnotify script by promag
  45. DrahtBot added the label Needs rebase on Jul 20, 2018
  46. DrahtBot commented at 10:39 PM on July 20, 2018: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  47. promag cross-referenced this on Jul 21, 2018 from issue ZMQ notices just for wallet-affecting TXes? by Crypto2
  48. DrahtBot commented at 1:41 PM on October 28, 2018: contributor

    <!--5fd3d806e98f4a0ca80977bb178665a0-->There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

  49. DrahtBot added the label Up for grabs on Oct 28, 2018
  50. DrahtBot closed this on Oct 28, 2018
  51. jonasschnelli cross-referenced this on Dec 9, 2018 from issue zmq: Add jsonblock, jsontx by ch4ot1c
  52. laanwj removed the label Needs rebase on Oct 24, 2019
  53. bitcoin locked this on Dec 16, 2021
Contributors
jonasschnellipromagEmpactDrahtBot
Labels
WalletRPC/REST/ZMQUp for grabs
Linked (view graph)
#7949 [RPC] Add RPC long poll notifications#13237 [Wallet Feature Request]: Multiwallet-compatible walletnotify#13315 configurable ZMQ message limit#13339 wallet: Replace %w by wallet name in -walletnotify script#13427 ZMQ notices just for wallet-affecting TXes?#14876 zmq: Add jsonblock, jsontx
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: 2026-05-20 06:55 UTC