[RPC] Only return hex field once in getrawtransaction #11027

achow101 commented at 11:08 PM on August 10, 2017: member

The hex is already returned in TxToUniv(), no need to give it out a second time in getrawtransaction itself.

Only return hex field once in getrawtransaction

The hex is already returned in TxToUniv, no need to give it out a
second independent time in getrawtransaction itself.

e029c6e709

fanquake added the label RPC/REST/ZMQ on Aug 10, 2017

sdaftuar commented at 12:21 PM on August 11, 2017: member

ACK

MarcoFalke commented at 1:21 PM on August 11, 2017: member

utACK e029c6e, as this is a bug fix. Though, could this potentially break client when -rpcserialversion=0? I am thinking about how different json implementations handle duplicate keys in a dict, where each key points to a different value.

achow101 commented at 6:54 PM on August 11, 2017: member

@MarcoFalke Hmm. That might be a problem since RPCSerializationFlags are not passed to EncodeHexTx in TxToUniv.

sipa commented at 7:00 PM on August 11, 2017: member

That might be a problem since RPCSerializationFlags are not passed to EncodeHexTx in TxToUniv.

That seems to be a bug on its own.

achow101 commented at 7:05 PM on August 11, 2017: member

Another thing is that not everything that uses TxToUniv needs to have hex output, e.g. decoderawtransaction

achow101 commented at 7:22 PM on August 11, 2017: member

I added a commit which passes the RPCSerializationFlags to TxToUniv and a boolean for whether to give hex output or not.

in src/core_io.h:34 in eb1dcaa52a outdated

  30 | @@ -31,6 +31,6 @@ UniValue ValueFromAmount(const CAmount& amount);
  31 |  std::string FormatScript(const CScript& script);
  32 |  std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
  33 |  void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
  34 | -void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry);
  35 | +void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, const int serializeFlags = 0, bool include_hex = true);

sipa commented at 8:01 PM on August 11, 2017:

Marking a by-value parameter as const is redundant. Also, style nit on serializeFlags.

achow101 commented at 8:11 PM on August 11, 2017:

Fixed. It was copy and pasted from two lines above in EncodeHexTx :)

sipa commented at 8:03 PM on August 11, 2017: member

utACK

achow101 force-pushed on Aug 11, 2017

in src/core_io.h:34 in 9381d8625b outdated

  30 | @@ -31,6 +31,6 @@ UniValue ValueFromAmount(const CAmount& amount);
  31 |  std::string FormatScript(const CScript& script);
  32 |  std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
  33 |  void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
  34 | -void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry);

promag commented at 11:59 PM on August 11, 2017:

I think the order should be inverted. There is no need to pass serialize_flags if one wants to include_hex=false.

laanwj commented at 4:33 PM on August 17, 2017:

~~Don't care about the order, though I'd prefer not using default arguments. Easy enough to just add these on all call sites, there aren't many.~~

Edit: never mind, I think this is a good use of default arguments, just swap them as @promag says.

MarcoFalke commented at 12:18 AM on August 12, 2017: member

That seems to be a bug on its own.

I don't think so?

According to the doc, -rpcserialversion sets the serialization of raw transaction or block hex returned in non-verbose mode.

promag commented at 12:19 AM on August 12, 2017: member

utACK 9381d86.

There are only 2 more calls to TxToUniv so I wonder if it pays to have default arguments.

achow101 commented at 12:28 AM on August 12, 2017: member

@MarcoFalke I interpret that as talking about when verbose mode means that you wouldn't get the hex back, e.g. in getblock. If we are returning hex, then I think we should be consistent and all hex should follow whatever the RPCSerializationFlags are.

gmaxwell approved

gmaxwell commented at 8:32 PM on August 14, 2017: contributor

ACK. This needs a 0.15 tag: it's a 0.15 regression and it results in invalid JSON.

sipa added the label Bug on Aug 14, 2017

sipa added this to the milestone 0.15.0 on Aug 14, 2017

MarcoFalke added the label Needs backport on Aug 16, 2017

jnewbery commented at 5:19 PM on August 17, 2017: member

Strictly speaking, I don't think this is invalid JSON, although client behaviour is undefined if object names are not unique (https://tools.ietf.org/html/rfc7159#section-4). But yes, this is a regression in 0.15 and should be treated as a bug fix.

Perhaps we should add JSON object checking to rpcserver.cpp (in JSONRPCExecOne()) and/or authproxy.py (in __call__()) to verify the validity of our returned JSON.

jnewbery commented at 5:33 PM on August 17, 2017: member

Concept ACK

Another thing is that not everything that uses TxToUniv needs to have hex output, e.g. decoderawtransaction I added a commit which passes ... a boolean for whether to give hex output or not.

I don't think you should complicate the interface to TxToUniv to specify which fields to return. Either filter them out at the caller, or just return the hex in decoderawtransaction. Is there any harm in returning the hex in that RPC?

Pass serialization flags and whether to include hex to TxToUniv 6bbdafcdc4

achow101 commented at 5:52 PM on August 17, 2017: member

Swapped the arguments.

Either filter them out at the caller

I looked at that but I couldn't find anything in Univalue for removing things from it.

or just return the hex in decoderawtransaction. Is there any harm in returning the hex in that RPC?

Since you are already providing it the hex, it's not useful to be giving the hex back out to the user.

achow101 force-pushed on Aug 17, 2017

laanwj added this to the "Blockers" column in a project

laanwj removed this from the "Blockers" column in a project

jtimon commented at 8:41 PM on August 17, 2017: contributor

utACK e029c6e

There are only 2 more calls to TxToUniv so I wonder if it pays to have default arguments.

+1

promag commented at 10:54 PM on August 17, 2017: member

Wild option 🙄:

void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry);
void TxToUnivWithHex(const CTransaction& tx, const uint256& hashBlock, int serialize_flags, UniValue& entry);

laanwj commented at 6:41 AM on August 21, 2017: member

Since you are already providing it the hex, it's not useful to be giving the hex back out to the user.

I was also really surprised to see the hex being echoed back in decoderawtransaction output. That's just a waste of bandwidth especially for large transactions.

Either filter them out at the caller

No, don't do this. univalue objects aren't meant to be written to/processed after initial generation. The library is not optimized for that. If you need any kind of editing use an intermedaite data structure (which makes no sense from 'XXXToJSON` functions). Specifying which fields to include looks like exactly the right way here.

laanwj merged this on Aug 21, 2017

laanwj closed this on Aug 21, 2017

laanwj referenced this in commit 820ddd48a7 on Aug 21, 2017

laanwj referenced this in commit eb9c21ed79 on Aug 21, 2017

laanwj referenced this in commit 07164bbead on Aug 21, 2017

laanwj removed the label Needs backport on Aug 21, 2017

achow101 deleted the branch on Aug 29, 2017

jasonbcox referenced this in commit e3b6fe0ed1 on Mar 26, 2019

jonspock referenced this in commit 60825429db on Apr 6, 2019

jonspock referenced this in commit 3740293492 on Apr 8, 2019

jonspock referenced this in commit 251d8051e1 on Apr 8, 2019

PastaPastaPasta referenced this in commit 4800913587 on Sep 19, 2019

codablock referenced this in commit 786dc78edf on Jan 21, 2020

UdjinM6 referenced this in commit 1dd2b11000 on Jan 22, 2020

ckti referenced this in commit 5d9389fc0f on Mar 28, 2021

gades referenced this in commit 10dc0aca56 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021