Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
promag
force-pushed
the
2019-01-zmqpubwallettx
branch
from
0839a12 to
52a5dd8
Compare
| uint32_t name_size = htonl(wallet->GetName().size()); | ||
| oss.write((const char*)&name_size, sizeof(name_size)); | ||
| oss << wallet->GetName(); | ||
| oss.write((const char*) hash.begin(), 32); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Ah right, sequence is already a different part. Will update accordingly.
|
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. |
|
Also note I have a recent (0.19) rebase of the old PR here: 8db19e8 |
6 participants
This is an alternative to
-walletnotifywhich 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.pyas well ascontrib/zmq/zmq_sub.pyand relevant documentation.