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

jonasschnelli · 2018-05-17T11:56:34Z

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

promag

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.

promag · 2018-05-17T13:08:05Z

src/wallet/wallet.cpp

nit, space after if.

promag · 2018-05-17T13:09:40Z

src/wallet/wallet.cpp

Can this be nullptr? Could assert(pwtx) instead?

promag · 2018-05-17T13:18:29Z

src/wallet/wallet.h

Could keep wtxOrdered back iterator instead:

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

Then above could be:

if (m_latest_wtxid != wtxOrdered.rbegin()) { ... }

I prefer to keep a non references object in this case.

promag · 2018-05-17T13:19:39Z

src/wallet/wallet.cpp

... = wtxOrdered.rbegin()->second.first;

promag · 2018-05-17T13:20:23Z

src/wallet/wallet.cpp

Missing notify on m_cond_txadd?

promag · 2018-05-17T13:22:14Z

src/wallet/rpcwallet.cpp

txid is required, should be out of ( ).

promag · 2018-05-17T13:24:01Z

src/wallet/rpcwallet.cpp

Should be

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

which is the error used in other calls.

promag · 2018-05-17T13:24:20Z

src/wallet/rpcwallet.cpp

nit move up

clang-format would fix a few other minor things as well

promag · 2018-05-17T13:24:46Z

src/wallet/rpcwallet.cpp

arg name should be poll?

Fixed the above; polltime is correct

promag · 2018-05-17T13:25:16Z

src/wallet/rpcwallet.cpp

Remove this invalid usage?

jonasschnelli · 2018-05-17T16:42:43Z

Fixed points found by @promag, ran clang-format
Added an example python script that shows the push channel capabilities

promag

Will play around, just some nits that could be fixed meanwhile.

promag · 2018-05-17T16:44:13Z

src/wallet/rpcwallet.cpp

promag · 2018-05-17T16:44:20Z

src/rpc/client.cpp

promag · 2018-05-17T16:44:43Z

contrib/push/example_walletnotify.py

nit, add newline.

promag · 2018-05-17T16:53:59Z

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

jonasschnelli · 2018-05-17T17:00:12Z

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

promag

Another round.

promag · 2018-05-17T17:18:18Z

contrib/push/example_walletnotify.py

Nit, space example code.

promag · 2018-05-17T17:19:00Z

contrib/push/example_walletnotify.py

promag · 2018-05-17T17:19:37Z

contrib/push/example_walletnotify.py

nit, sort these.

promag · 2018-05-17T17:21:30Z

src/wallet/rpcwallet.cpp

greater than zero

I don't understand that comment.

s/then/than: If greater than zero, wait with ...

promag · 2018-05-17T17:23:36Z

src/wallet/rpcwallet.cpp

Why lock cs_main?

IMO cs_main should always be locked before cs_wallet, especially with the usage of ListTransactions() and its subclass.

promag · 2018-05-17T17:24:02Z

src/wallet/rpcwallet.cpp

Could have a limit argument? Otherwise it would dump the whole wallet?

I thought of it,... but would like to keep it simple for a first implementation (limit could follow next)

promag · 2018-05-17T17:27:58Z

src/wallet/rpcwallet.cpp

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)

DrahtBot · 2018-07-20T22:39:05Z

Needs rebase

DrahtBot · 2018-10-28T13:41:07Z

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.

jonasschnelli added Wallet RPC/REST/ZMQ labels May 17, 2018

jonasschnelli force-pushed the 2018/05/listsincetx branch from 89661ee to ecb2e20 Compare May 17, 2018 12:42

promag reviewed May 17, 2018

View reviewed changes

jonasschnelli changed the title ~~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 May 17, 2018

jonasschnelli force-pushed the 2018/05/listsincetx branch from ecb2e20 to d68f662 Compare May 17, 2018 16:30

Keep track of the latest wtxid

c36e008

jonasschnelli force-pushed the 2018/05/listsincetx branch from d68f662 to 139ade1 Compare May 17, 2018 16:36

promag reviewed May 17, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/05/listsincetx branch 2 times, most recently from 51ea1fd to 2f505ac Compare May 17, 2018 16:52

promag reviewed May 17, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/05/listsincetx branch from 2f505ac to 3f1b0a1 Compare May 17, 2018 18:40

jonasschnelli added 3 commits May 17, 2018 20:40

RPC: add listsincetx, a way to longpoll for new wallet transactions

7fa78fa

QA: add ListSinceTxTest test

a6535bb

Contrib: add walletnotify example code

2dadbc8

jonasschnelli force-pushed the 2018/05/listsincetx branch from 3f1b0a1 to 2dadbc8 Compare May 17, 2018 18:42

This was referenced May 23, 2018

Possibly missing updates from Bitcoind backend lightningnetwork/lnd#1174

Closed

configurable ZMQ message limit #13315

Closed

wallet: Replace %w by wallet name in -walletnotify script #13339

Merged

DrahtBot added the Needs rebase label Jul 20, 2018

promag mentioned this pull request Jul 21, 2018

ZMQ notices just for wallet-affecting TXes? #13427

Closed

DrahtBot added the Up for grabs label Oct 28, 2018

DrahtBot closed this Oct 28, 2018

jonasschnelli mentioned this pull request Dec 9, 2018

zmq: Add jsonblock, jsontx #14876

Closed

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

jonasschnelli commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 17, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 17, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jonasschnelli commented May 17, 2018 •

edited

Loading

promag commented May 17, 2018 •

edited

Loading

promag May 17, 2018 •

edited

Loading