Patch CTID

[dangell7]

High Level Overview of Change

Fix a number of issues involved with CTID.

1. CTID is not present on all RPC tx transactions.
2. rpcWRONG_NETWORK was missing in the ErrorCodes.cpp

• FIx https://github.com/XRPLF/rippled/issues/4735

Context of Change

• Initial CTID support added in: #4418
• XLS-37 specification

Type of Change

• [x] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Refactor (non-breaking change that only restructures code)
• [x] Tests (you added tests for code that already exists, or your new feature included in this PR)
• [ ] Documentation update
• [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• [ ] Release

API Impact

• [x] Public API: New feature (new methods and/or new fields)
• [ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• [ ] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• [ ] Peer protocol change (must be backward compatible or bump the peer protocol version)

[cindyyan317]

I ran this PR locally. I can see there is ctid field in subscription and account_tx now. However, i can not see ctid in tx response any more. My input: ''' { "method": "tx", "params":[{ "transaction": "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428"}] } ''' output:

"ctid" is available for the same transaction from account_tx response.

[dangell7]

I ran this PR locally. I can see there is ctid field in subscription and account_tx now. However, i can not see ctid in tx response any more. My input: ''' { "method": "tx", "params":[{ "transaction": "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428"}] } ''' output:

"ctid" is available for the same transaction from account_tx response.

Whats the networkID? Can you show me the json for the response that was correct?

[cindyyan317]

I ran this PR locally. I can see there is ctid field in subscription and account_tx now. However, i can not see ctid in tx response any more. My input: ''' { "method": "tx", "params":[{ "transaction": "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428"}] } ''' output: "ctid" is available for the same transaction from account_tx response.

Whats the networkID? Can you show me the json for the response that was correct?

I should mention I am on devnet. I think the correct one should contain the "ctid", right?

[dangell7]

I ran this PR locally. I can see there is ctid field in subscription and account_tx now. However, i can not see ctid in tx response any more. My input: ''' { "method": "tx", "params":[{ "transaction": "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428"}] } ''' output: "ctid" is available for the same transaction from account_tx response.

Whats the networkID? Can you show me the json for the response that was correct?

I should mention I am on devnet. I think the correct one should contain the "ctid", right?

It should... I will review this again to make sure.

[dangell7]

@intelliot I believe devent was reset to a networkID < 1024 therefore it did not have a ctid. @cindyyan317 can you please recheck the devnet again with your locally running PR?

[cindyyan317]

@cindyyan317 can you please recheck the devnet again with your locally running PR?

Hi @dangell7 . I can see the ctid field from tx for some transactions. eg "6435CFB5597BDD5F830847EC95EF3840AD3C21F47A224D9FDB62654AA693F2D8" 's ctid is available, but "0609E53FF80DFA2106B46AA2BE6E04A06F5C74AA445CC89ED6F6005F995792E8" is not.

for the transaction "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428", i still can not see its ctid from tx. However , i can see its ctid from account_tx output.

@intelliot I believe devent was reset to a networkID < 1024 therefore it did not have a ctid.

What does it mean? It should not have ctid when networdID < 1024? devnet's networkId has been always 2 , right?

[intelliot]

Yes, I think Devnet's NetworkID has always been 2.

@dangell7 , I think it should be possible (and would be ideal) to always return ctid with validated transactions, for any NetworkID (even if it's <= 1024).

[dangell7]

Yes, I think Devnet's NetworkID has always been 2.

@dangell7 , I think it should be possible (and would be ideal) to always return ctid with validated transactions, for any NetworkID (even if it's <= 1024).

You're right and I will look at the issue deeper this weekend. I don't like peering from my work computer. I have an env now to spin up a peer easier to test.

I will also check on the CTID on txs <= 1024. I think I misspoke.

[dangell7]

@intelliot moving this to draft while I write more tests. Can confirm ctid is returned when network id <= 1024.

[dangell7]

@intelliot I'm opening this back up for review. I see no issues

[intelliot]

@dangell7 - I just updated your PR description to use the PR description template. Please feel free to edit / add more. Also, consider the suggestions from scottschurr above, and comment here when you are done.

[dangell7]

@scottschurr Thank you very much for the review. I looked over the code and everything looks fine. I'm going to run the tests and build a version to test locally.

Thank you

[dangell7]

@scottschurr Thanks again for your update. I have fixed all of the issues. However, we are getting some weird behavior from this. Its sometimes on the tx and sometimes not. We are chasing this down now.

I believe more tests need to be written for this or a bug needs to be fixed. As soon as we can replicate the issue I will fix this.

[intelliot]

Given

However, we are getting some weird behavior from this. Its sometimes on the tx and sometimes not. We are chasing this down now.

Could you Convert to draft this PR so we know it isn't ready for merging right now?

Of course, when you do think it may be ready, please mark it as "ready for review".

[intelliot]

Internal tracker: RPFC-82

[intelliot]

note: @dangell7 indicated he will push changes tomorrow.

[intelliot]

• awaiting update or comment from @dangell7

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 97.61905% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 71.0%. Comparing base (244ac5e) to head (47df4c2).

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #4738   +/-   ##
=======================================
  Coverage     71.0%   71.0%           
=======================================
  Files          796     796           
  Lines        66793   66820   +27     
  Branches     10987   10986    -1     
=======================================
+ Hits         47420   47442   +22     
- Misses       19373   19378    +5     

Files	Coverage Δ
src/ripple/app/ledger/impl/TransactionMaster.cpp	71.2% <100.0%> (ø)
src/ripple/app/misc/NetworkOPs.cpp	65.9% <100.0%> (+0.2%)	:arrow_up:
src/ripple/app/misc/Transaction.h	85.2% <ø> (ø)
src/ripple/app/misc/impl/Transaction.cpp	76.5% <100.0%> (+4.5%)	:arrow_up:
src/ripple/app/rdb/backend/detail/impl/Node.cpp	56.8% <100.0%> (+0.1%)	:arrow_up:
src/ripple/protocol/impl/ErrorCodes.cpp	71.4% <ø> (ø)
src/ripple/rpc/CTID.h	100.0% <100.0%> (ø)
src/ripple/rpc/handlers/Tx.cpp	68.7% <100.0%> (+2.2%)	:arrow_up:
src/ripple/app/misc/impl/AccountTxPaging.cpp	75.0% <80.0%> (-1.9%)	:arrow_down:

... and 5 files with indirect coverage changes

[intelliot]

• there is some missing unit test coverage (see above)
• @dangell7 to let us know when this is, from his perspective, ready to merge

[dangell7]

@intelliot working on the codecov

[intelliot]

@dangell7 how would you like this to move forward?

[dangell7]

@seelabs Yeah this is ready. I tried to test the convertBlobsToTxResult but was unable to build a good testcase without rewriting the function.

[intelliot]

seelabs confirmed that this still needs perf testing. cc @sophiax851

[sophiax851]

I tried to build a rippled with this PR to run on the devnet's full history node, but since this PR was based off the 2.2.0-rc1, so the build is getting amendment blocked. @dangell7 can you please rebase off the master branch so I can build it for 2.2.0? Thanks.