wip: zmq: Support -zmqpubwallettx #17878

pull promag wants to merge 4 commits into bitcoin:master from promag:2019-01-zmqpubwallettx changing 10 files +71 −3

promag commented at 3:37 AM on January 6, 2020: member

This is an alternative to -walletnotify which has the advantage of publishing the wallet name. This is essential in a multi-wallet setup.

I'm submitting to seek concept ACK and/or suggestions.

Currently the published message contains the wallet name (lengh + bytes) followed by 32 bytes of the wallet transaction. Also considering to include the change (added, updated, removed) but I'm open for suggestions.

Currently working on extending the test test/functional/interface_zmq.py as well as contrib/zmq/zmq_sub.py and relevant documentation.
wallet: Add g_wallet_transaction_changed signal 58fc12755f
zmq: Support -zmqpubwallettx b9059ccb10
fanquake added the label RPC/REST/ZMQ on Jan 6, 2020
fanquake added the label Needs Conceptual Review on Jan 6, 2020
promag cross-referenced this on Jan 6, 2020 from issue wallet: Replace %w by wallet name in -walletnotify script by promag
DrahtBot commented at 8:22 AM on January 6, 2020: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
- #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
- #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
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.
zmq: Enable -zmqpubwallettx aed8034a90
contrib: Subscribe to wallettx in zmq_sub.py 52a5dd871a
promag force-pushed on Jan 6, 2020
in src/zmq/zmqpublishnotifier.cpp:219 in 52a5dd871a
```
 214 | +{
 215 | +    std::ostringstream oss;
 216 | +    uint32_t name_size = htonl(wallet->GetName().size());
 217 | +    oss.write((const char*)&name_size, sizeof(name_size));
 218 | +    oss << wallet->GetName();
 219 | +    oss.write((const char*) hash.begin(), 32);
```
0xB10C commented at 12:36 PM on January 6, 2020:

Concept alternative: ZMQ supports mulitpart messages. Might make sense to split the wallet name, hash and reason over multiple frames rather than creating a custom protocol for serialization.

promag commented at 12:38 PM on January 6, 2020:

I think we can't do that unless the payload goes after the sequence number - I mean if we want to be consistent with existing messages.
0xB10C commented at 12:57 PM on January 6, 2020:

Agree, that it would not be super consistent with the existing messages. However, there was just never the need for more that one payload.

For discussion: Currently we send a fixed 3 part multipart message.

| topic | payload | sequence number |

I had an extension similar the following in mind when I read you PR description.

| topic | wallet name | hash | (reason/change) | sequence number |
promag commented at 6:18 PM on January 6, 2020:

Ah right, sequence is already a different part. Will update accordingly.
bitcoin deleted a comment on Jan 9, 2020
laanwj added this to the "Chasing Concept ACK" column in a project

fanquake added this to the "In progress" column in a project

luke-jr commented at 7:07 PM on January 26, 2020: member

See also #10554. This older PR is apparently both used in production by its author, @somdoron, as well as included in Bitcoin Knots. For that reason, it would be ideal to use an interface compatible with it if reasonably possible - I think simply putting the hash as the first item might work; maybe multipart too? (not sure)

Note it's called -zmqpubhashwallettx there (and a raw variant provided). I'm not sure if the raw variant is as easily extended to include a wallet name - that might require the multipart stuff.
luke-jr commented at 7:10 PM on January 26, 2020: member

Also note I have a recent (0.19) rebase of the old PR here: https://github.com/bitcoin/bitcoin/commit/8db19e8ccfd1740cd57b006dfc2387926ade7a6b
promag commented at 7:11 PM on January 26, 2020: member

@luke-jr thanks for pointing that, I was under the impression there was something similar. I'll look closely at 8db19e8.
fanquake removed this from the "Chasing Concept ACK" column in a project

promag cross-referenced this on May 8, 2020 from issue ZMQ notices just for wallet-affecting TXes? by Crypto2
promag closed this on Nov 1, 2020
promag deleted the branch on Nov 1, 2020
bitcoin locked this on Feb 15, 2022

Contributors

Labels

RPC/REST/ZMQ Needs Conceptual Review

Linked (view graph)

#10554 ZMQ: add publishers for wallet transactions.#13339 wallet: Replace %w by wallet name in -walletnotify script #13427 ZMQ notices just for wallet-affecting TXes?

wip: zmq: Support -zmqpubwallettx #17878

Conflicts