refactor: reunify common fields

[mDuo13]

Makes transactions and pseudo-transactions share the same commonFields again. This change technically allows pseudo-transactions to have a TicketSequence field, but those transactions are only ever constructed by code paths that don't add such a field, so this is not actually a transaction processing change.

(Note, my build is not working at the moment, but the amount I was able to build so far gets past this file at least.)

Context of Change

Fixes #4714

Type of Change

• Refactor (non-breaking change that only restructures code)

Future Tasks

We could optionally add a separate check to make sure this and other fields that don't make sense on pseudo-transactions are never added to them, but I don't think that's necessary.

[seelabs]

I think this is a good change. It is a little strange to add fields that should never be present in those transactions, but it regularizes the code in a nice way. 👍

[intelliot]

Suggested commit message:

refactor: reunify transaction common fields: (#4715)

Make transactions and pseudo-transactions share the same commonFields
again. This regularizes the code in a nice way.

While this technically allows pseudo-transactions to have a
TicketSequence field, pseudo-transactions are only ever constructed by
code paths that don't add such a field, so this is not a transaction
processing change. It may be possible to add a separate check to ensure
TicketSequence (and other fields that don't make sense on
pseudo-transactions) are never added to pseudo-transactions, but that
should not be necessary. (TicketSequence is not the only common field
that can not and does not appear in pseudo-transactions.) Note:
TicketSequence is already documented as a common field.

Related: #4637

Fix #4714

[intelliot]

With the way transactions' canonical format is defined, this refactor should not change the actual format of any transactions. Thus, it does not require an amendment. That said, this refactor changes the order in which fields are added to transaction types in TxFormats.cpp. To test that the order does not matter, one can serialize a transaction with TicketSequence to binary on a server with the code change, and a server without it; if the two are identical, then that confirms this is not a transaction processing change. For complete certainty, it should be on a transaction type that defines a field after TicketSequence and should use that field. That would be an AccountSet with NFTokenMinter or an AMMDeposit with TradingFee.

@SaxenaKaustubh @legleux @manojsdoshi

[SaxenaKaustubh]

Is there any testing done on this PR? Is there any test plan?

[intelliot]

Discussed with @SaxenaKaustubh and @manojsdoshi and decided on a test plan:

1. Merge to develop branch and test binaries.
2. Ensure we have integration tests which test transactions with TicketSequence and use fields after TicketSequence: AccountSet with NFTokenMinter and AMMDeposit with TradingFee.
3. Test network with 4 validators running master and 1 running develop and run e2e tests, with all tx submitted to the validator running develop (with this PR).

[ximinez]

With the way transactions' canonical format is defined, this refactor should not change the actual format of any transactions. Thus, it does not require an amendment. That said, this refactor changes the order in which fields are added to transaction types in TxFormats.cpp. To test that the order does not matter, one can serialize a transaction with TicketSequence to binary on a server with the code change, and a server without it; if the two are identical, then that confirms this is not a transaction processing change. For complete certainty, it should be on a transaction type that defines a field after TicketSequence and should use that field. That would be an AccountSet with NFTokenMinter or an AMMDeposit with TradingFee.

Ok, I did this experiment. I used rippled 1.12.0 to sign a bogus transaction, took the json and serialized output of that signing, then serialized and deserialized each respectively using a branch of the ripple-offline-tool built against 2.0.0-rc5, then compared the output of the two to determine they're identical.

$ rippled -q server_info | jq '.result.info.build_version'
"1.12.0+89780c8e4fd4d140fcb912cf2d0c01c1b260539e.DEBUG"

$ cat transaction.txt 
{
  "TransactionType" : "AccountSet",
  "Flags" : 2148532224,
  "SourceTag" : 123,
  "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
  "Sequence" : 0,
  "Fee" : 1000,
  "TicketSequence" : 43000,
  "NetworkID" : 1,
  "SetFlag" : 7,
  "TickSize" : 4
}

$ echo $( cat transaction.txt  ) | tee transaction1.txt
{ "TransactionType" : "AccountSet", "Flags" : 2148532224, "SourceTag" : 123, "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", "Sequence" : 0, "Fee" : 1000, "TicketSequence" : 43000, "NetworkID" : 1, "SetFlag" : 7, "TickSize" : 4 }

$ rippled -q sign s1234 "$( cat transaction1.txt )" offline | jq -r '.result.tx_json' | tee transaction2.json.txt 
{
  "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
  "Fee": "1000",
  "Flags": 2148532224,
  "NetworkID": 1,
  "Sequence": 0,
  "SetFlag": 7,
  "SigningPubKey": "0208C9236CB71959259FD720ED939BB42D41F67F2893CC05F310C64E483CC43676",
  "SourceTag": 123,
  "TickSize": 4,
  "TicketSequence": 43000,
  "TransactionType": "AccountSet",
  "TxnSignature": "3045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A4",
  "hash": "FD331D790B434B2A2DE1FA7C3061D3E15B3F578418790B0FF3B8642DCB7A77C6"
}

$ rippled -q sign s1234 "$( cat transaction1.txt )" offline | jq -r '.result.tx_blob' | tee transaction2.blob.txt 
12000321000000012280100000230000007B240000000020210000000720290000A7F86840000000000003E873210208C9236CB71959259FD720ED939BB42D41F67F2893CC05F310C64E483CC4367674473045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A48114B5F762798A53D543A014CAF8B297CFF8F2F937E800101004

$ cat transaction2.json.txt | ./build/cmake/gcc.Debug.ON/ripple-offline-tool serialize --stdin | tee transaction3.blob.txt
12000321000000012280100000230000007B240000000020210000000720290000A7F86840000000000003E873210208C9236CB71959259FD720ED939BB42D41F67F2893CC05F310C64E483CC4367674473045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A48114B5F762798A53D543A014CAF8B297CFF8F2F937E800101004

$ cat transaction2.blob.txt | ./build/cmake/gcc.Debug.ON/ripple-offline-tool deserialize --stdin | tee transaction3.json.txt
{
   "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
   "Fee" : "1000",
   "Flags" : 2148532224,
   "NetworkID" : 1,
   "Sequence" : 0,
   "SetFlag" : 7,
   "SigningPubKey" : "0208C9236CB71959259FD720ED939BB42D41F67F2893CC05F310C64E483CC43676",
   "SourceTag" : 123,
   "TickSize" : 4,
   "TicketSequence" : 43000,
   "TransactionType" : "AccountSet",
   "TxnSignature" : "3045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A4"
}

$ diff transaction*.blob.txt

$ diff -w transaction*.json.txt13,14c13
<   "TxnSignature": "3045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A4",
<   "hash": "FD331D790B434B2A2DE1FA7C3061D3E15B3F578418790B0FF3B8642DCB7A77C6"
---
>    "TxnSignature" : "3045022100850F4CAD7E14542CF6DF72CFB6C00FA3447CE571537A6C7494C74EB4B52479300220469C38AB38AE9915BF66008928FD987A7588AE60F88ECB31A4705BE649EDF9A4"
15a15
> 

(Note the only difference in the json is the absent hash field, which is expected, and the , at the end of the TxnSignature line.)

[ckeshava]

@ximinez thank you very much for running this experiment. The ripple-offline-tool is very convenient!
I have independently verified the signing of the transaction using a local binary of rippled at the latest tip of the develop branch. Can I deserialize the tx_blob without using the ripple-offline-tool? I'm searching for command-line tools that help me deserialize, but I couldn't find any.

@Bronek The rippled sign command returns the hash field inside the tx_json. I observed this in the tip of develop branch. I remember that #4727 purports to change the location of the hash field. Do we need to change the output of the sign command?

[Bronek]

@Bronek The rippled sign command returns the hash field inside the tx_json. I observed this in the tip of develop branch. I remember that #4727 purports to change the location of the hash field. Do we need to change the output of the sign command?

Currently ripple sign and all other command-line APIs are set to API version 1.0 , so they maintain the "old" format. We plan to bump apiCommandLineVersion at some point in near future, that will change the output format.