wip: zmq: Support -zmqpubwallettx #17878

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

2. wallet: Add g_wallet_transaction_changed signal 58fc12755f
3. zmq: Support -zmqpubwallettx b9059ccb10
4. fanquake added the label RPC/REST/ZMQ on Jan 6, 2020
5. fanquake added the label Needs Conceptual Review on Jan 6, 2020
6. 
   DrahtBot commented at 8:22 am on January 6, 2020: member

   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.

7. zmq: Enable -zmqpubwallettx aed8034a90
8. contrib: Subscribe to wallettx in zmq_sub.py 52a5dd871a
9. promag force-pushed on Jan 6, 2020
10. 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.

    0| topic | payload | sequence number |
    

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

    0| 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.
11. fanquake deleted a comment on Jan 9, 2020
12. laanwj added this to the "Chasing Concept ACK" column in a project

    ---

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

    ---

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

15. 
    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
16. 
    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.
17. fanquake removed this from the "Chasing Concept ACK" column in a project

    ---

18. promag closed this on Nov 1, 2020

    ---

19. promag deleted the branch on Nov 1, 2020
20. DrahtBot locked this on Feb 15, 2022

Contributors
promag DrahtBot 0xB10C luke-jr

Labels
RPC/REST/ZMQ Needs Conceptual Review