ZMQ: Add decodedtx topic for JSON tx publishing #11614

astanway commented at 12:37 AM on November 6, 2017: none

Adds a new ZMQ topic called decodedtx that uses TxToUniv to decode all newly arrived transaction into JSON and publish them over ZMQ.
Includes functional tests

ZMQ: Add decodedtx topic for JSON tx publishing 0568067ba0

fanquake added the label RPC/REST/ZMQ on Nov 6, 2017

in src/zmq/zmqpublishnotifier.cpp:212 in 0568067ba0 outdated

 207 | +    UniValue result(UniValue::VOBJ);
 208 | +    TxToUniv(transaction, uint256(), result, false);
 209 | +    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
 210 | +    ss << result.write();
 211 | +    return SendMessage(MSG_DECODEDTX, &(*ss.begin()), ss.size());
 212 | +}

promag commented at 12:49 AM on November 6, 2017:

Add newline.

in test/functional/zmq_test.py:93 in 0568067ba0 outdated

  89 | @@ -88,6 +90,9 @@ def _zmq_test(self):
  90 |          self.sync_all()
  91 |  
  92 |          for x in range(num_blocks):
  93 | +            # Should receive the json broadcasted raw transaction.

promag commented at 12:50 AM on November 6, 2017:

Remove raw? Same below.

in src/zmq/zmqpublishnotifier.cpp:208 in 0568067ba0 outdated

 203 | +bool CZMQPublishDecodedTransactionNotifier::NotifyTransaction(const CTransaction &transaction)
 204 | +{
 205 | +    uint256 hash = transaction.GetHash();
 206 | +    LogPrint(BCLog::ZMQ, "zmq: Publish decodedtx %s\n", hash.GetHex());
 207 | +    UniValue result(UniValue::VOBJ);
 208 | +    TxToUniv(transaction, uint256(), result, false);

promag commented at 12:52 AM on November 6, 2017:

Have you tried TxToJSON?

astanway commented at 2:21 AM on November 6, 2017:

TxToJSON just calls TxToUniv internally, and also adds that extra hashBlock check, which is not strictly necessary imo. If we think that extra info would be useful though (confirmations, time, blocktime, blockhash), we can try to get the block hash beforehand and switch to using this method.

in src/zmq/zmqpublishnotifier.cpp:12 in 0568067ba0 outdated

   8 | @@ -9,13 +9,17 @@
   9 |  #include "validation.h"
  10 |  #include "util.h"
  11 |  #include "rpc/server.h"
  12 | +#include "rpc/protocol.h"

promag commented at 12:53 AM on November 6, 2017:

Nit, sort includes.

in src/zmq/zmqpublishnotifier.cpp:22 in 0568067ba0 outdated

  21 | -static const char *MSG_RAWTX     = "rawtx";
  22 | +static const char *MSG_HASHBLOCK     = "hashblock";
  23 | +static const char *MSG_HASHTX        = "hashtx";
  24 | +static const char *MSG_RAWBLOCK      = "rawblock";
  25 | +static const char *MSG_RAWTX         = "rawtx";
  26 | +static const char *MSG_DECODEDTX     = "decodedtx";

promag commented at 12:55 AM on November 6, 2017:

Nit, sort.

in test/functional/zmq_test.py:119 in 0568067ba0 outdated

 113 | @@ -109,6 +114,10 @@ def _zmq_test(self):
 114 |          payment_txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1.0)
 115 |          self.sync_all()
 116 |  
 117 | +        # Should receive the json broadcasted raw transaction.
 118 | +        decodedtx = self.decodedtx.receive()
 119 | +        assert_equal(payment_txid, json.loads(decodedtx[3:])['txid'])

promag commented at 12:55 AM on November 6, 2017:

Care to explain the offset?

astanway commented at 2:23 AM on November 6, 2017:

Looks like it was some kind of header added by CDataStream - I'm not entirely sure, tbh. I switched it over to a vanilla string and char array and the offset went away.

promag commented at 12:57 AM on November 6, 2017: member

Concept ACK and welcome!

Address nits and decode utf-8 to pass build c96e4fe391

Fix comment 740841e7df

jonasschnelli commented at 3:03 AM on November 6, 2017: contributor

Tend to NACK. One of the downsides of our RPC concept is that everything is based on JSON. JSON has serval downsides (parsing is in general a minefield).

IMO ZMQ should be JSON-free and ideally binary/net-serialized only.

Why not just listening via ZMQ and fetch the required data via RPC if a ZMQ notification arrives?

Or where is the benefit of having a decoded JSON rather then decoding it on the client side?

astanway commented at 3:10 AM on November 6, 2017: none

The benefit is much greater speed than through RPC, primarily because ZMQ can be configured to use a unix domain socket instead of TCP (which RPC currently does not support). In addition, to listen for hashtx via ZMQ and then make an additional RPC request with it seems like a waste, when the needed data could be sent right off the bat.

Re: decoding client side - by decoding with the same methods that RPC uses to decode, this guarantees that your output will be identical to the RPC output and as robust as the RPC output, as opposed to rolling your own decoder which might differ / be buggy / lack features, especially as time goes on and the RPC output is possibly changed.

dcousens commented at 3:43 AM on November 6, 2017: contributor

@astanway perhaps a better long term outcome is some kind of IPC socket instead of the RPC/ZMQ.

promag commented at 8:19 AM on November 6, 2017: member

as opposed to rolling your own decoder which might differ / be buggy / lack features, especially as time goes on and the RPC output is possibly changed

There are libraries for this already.

IMO rich notifications are fine, even though it is possible to have the same data thru RPC or some client library. With rawtx you just have to parse the JSON and for prototypes or simple applications it could be that simple. I would just add a note regarding performance vs rawtx.

laanwj commented at 8:54 AM on November 6, 2017: member

IMO ZMQ should be JSON-free and ideally binary/net-serialized only.

I tend to agree here. The point of ZMQ is that it's fast, and adding JSON overhead (formatting on the server, parsing on the client) negates that. Why doesn't your application process the raw transaction data instead?

as opposed to rolling your own decoder which might differ / be buggy / lack features, especially as time goes on and the RPC output is possibly changed.

I think you have things reversed there - the chance of JSON output changing under you is much larger than the binary encoding of transactions changing, as that would be a consensus change, which is necessarily advertised widely. Processing binary transaction data directly increases robustness of whatever you're building against upstream changes.

unix domain socket instead of TCP (which RPC currently does not support)

FYI there's "UNIX sockets support for RPC" #9919 (though it needs rebasing).

promag commented at 9:38 AM on November 6, 2017: member

FYI this was discussed here #6103 (comment).

This being an option and using the same RPC format I keep the concept ACK. IMO the overhead is not that much (or it shouldn't be). Also, everything is in place to easily add new notifiers.

astanway commented at 12:46 AM on November 10, 2017: none

As far as performance goes - if the option is not enabled, it has zero impact (as far as I can understand from the rest of ZMQ project).

I haven't yet really heard a compelling reason to not add it - I think it's a very convenient and useful feature and will enable a lot of people to start developing quickly, without needing to write their own transaction parser from scratch (difficult) or using RPC (slow).

dcousens commented at 1:38 AM on November 10, 2017: contributor

transaction parser from scratch (difficult)

There are libraries in nearly every language you would use. It is probably a 1-line function call.

The only reasonable advantage of the RPC format** is the extra information encoded into the JSON format, eg block information that would otherwise be an extra round trip.

astanway commented at 1:47 AM on November 10, 2017: none

There are libraries in nearly every language you would use. It is probably a 1-line function call.

Truly there are not; the state of transaction parsers is in pretty horrible shape and none really match the canonical RPC JSON output.

The only reasonable advantage of the RPC

Sorry, not sure what we're talking about - I am defending using this ZMQ extension, not RPC.

dcousens commented at 1:55 AM on November 10, 2017: contributor

I am defending using this ZMQ extension, not RPC.

Apologies, I meant the RPC JSON format. Edited my reply to fit that.

Truly there are not; the state of transaction parsers is in pretty horrible shape and none really match the canonical RPC JSON output.

What language(s) are you using? Transaction parsing is a relatively simple task, approximating about 50-120 lines of code (even if you do the endianness yourself).

luke-jr commented at 4:30 PM on November 10, 2017: member

This is too wasteful and pointless IMO. Just decode the raw transaction on the client side.

jnewbery commented at 8:33 PM on March 27, 2018: member

I'm a -0 on this, for the reasons from @jonasschnelli and @laanwj .

This doesn't seem to have garnered support. @astanway - are you happy to close this?

astanway commented at 10:05 PM on March 27, 2018: none

I still think it can be quite useful to a lot of folks who want to get started developing on top of the blockchain. There's no reason to force parsing on users when we can do a little bit of work to do it natively and in a simple (aka, readily both human and machine readable) format like JSON. However, if ease of application development is not a priority for the project, then sure - happy to close.

jnewbery commented at 11:05 AM on March 28, 2018: member

if ease of application development is not a priority...

Thanks @astanway , this isn't so much about priorities, as whether the concept has gathered support from other contributors. This PR has 5 concept NACKs from regular contributors, so it seems unlikely to get support for merge.

I've had plenty of my PRs NACKed in the past. It's part of the open source process.

jnewbery closed this on Mar 28, 2018

promag commented at 11:48 AM on March 28, 2018: member

Agree with closing.

Truly there are not; the state of transaction parsers is in pretty horrible shape and none really match the canonical RPC JSON output.

Perhaps you could help to improve this instead.

MarcoFalke locked this on Sep 8, 2021