Patch CTID #4738

dangell7 · 2023-10-02T17:09:00Z

High Level Overview of Change

Fix a number of issues involved with CTID.

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

FIx [rpcWRONG_NETWORK does not have error info ] (Version: [2.0.0-b1]) #4735

Context of Change

Initial CTID support added in: feat: support Concise Transaction Identifier (CTID) (XLS-37) #4418
XLS-37 specification

Type of Change

Bug fix (non-breaking change which fixes an issue)
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)
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

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 · 2023-10-03T09:33:56Z

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 · 2023-10-03T09:55:56Z

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 · 2023-10-03T16:57:30Z

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 · 2023-10-04T07:21:02Z

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 · 2023-10-05T07:13:06Z

@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 · 2023-10-05T11:49:40Z

@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 · 2023-10-06T04:58:16Z

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 · 2023-10-06T06:43:03Z

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 · 2023-10-06T07:39:28Z

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

dangell7 · 2023-10-31T12:31:43Z

@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 · 2023-11-02T05:23:29Z

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 · 2024-01-09T00:35:38Z

Internal tracker: RPFC-82

intelliot · 2024-01-17T19:19:21Z

note: @dangell7 indicated he will push changes tomorrow.

intelliot · 2024-02-01T06:30:19Z

awaiting update or comment from @dangell7

codecov-commenter · 2024-02-01T18:21:52Z

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%)`	⬆️
src/ripple/app/misc/Transaction.h	`85.2% <ø> (ø)`
src/ripple/app/misc/impl/Transaction.cpp	`76.5% <100.0%> (+4.5%)`	⬆️
src/ripple/app/rdb/backend/detail/impl/Node.cpp	`56.8% <100.0%> (+0.1%)`	⬆️
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%)`	⬆️
src/ripple/app/misc/impl/AccountTxPaging.cpp	`75.0% <80.0%> (-1.9%)`	⬇️

... and 5 files with indirect coverage changes

intelliot · 2024-02-02T22:38:02Z

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

scottschurr

👍 Looks good to me.

dangell7 · 2024-02-06T09:57:45Z

@intelliot working on the codecov

dangell7 · 2024-02-06T10:41:03Z

src/ripple/app/misc/impl/AccountTxPaging.cpp

    auto metaset =
        std::make_shared<TxMeta>(tr->getID(), tr->getLedger(), rawMeta);

+    // if properly formed meta is available we can use it to generate ctid
+    if (metaset->getAsObject().isFieldPresent(sfTransactionIndex))


@scottschurr Can you look at this? Wouldn't sfTransactionIndex always exist? If I do account_tx on a non validated ledger I get "Ledger not validated." SetStatus values are optional. So can we just call this every time?

Or how do I test account_tx with a tx that doesnt have a sfTransactionIndex?

@dangell7, thanks for asking. I can't immediately think of a situation where you could retrieve metadata for a transaction from a ledger and not have sfTransactionIndex filled in. However I prefer the conservative approach of verifying that the field is present before accessing it. Bugs happen. Doing checks like this can prevent a bug elsewhere from turning catastrophic.

Taking a step back, we ran for a few years without code coverage as part of CI. And, thanks to some great work by good folks, we started getting code coverage reports again recently. But we're still learning how to best apply the tool. There will be code that can't be exercised by reasonable tests (testing asserts anyone?). We have a new microscope and we've discovered that there are tiny mites living on our skin. Over time we'll learn to relax and know we don't have to check that each individual mite is harmless.

Having said that, if I were bound and determined to exercise the else condition here, I'd doing it by calling the convertBlobsToTxResult() function directly. That would allow me to pass in whatever I wanted in the rawMeta argument. This test would pass in rawMeta that omitted an sfTransactionIndex. I'd guess that getting that test in place might take a day or so of effort.

Awesome. Thank you for this response. I do like the code coverage stuff. I will take a look at how to get some more coverage on this. convertBlobsToTxResult sounds pretty simple. Thanks

intelliot · 2024-02-20T13:49:42Z

@dangell7 how would you like this to move forward?

seelabs

👍 The code looks good to me. @dangell7 is this ready to merge or are there still things that need to be resolved?

dangell7 · 2024-05-14T18:24:49Z

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

intelliot · 2024-05-20T17:47:24Z

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

sophiax851 · 2024-06-26T19:04:28Z

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.

RichardAH and others added 6 commits October 2, 2023 13:58

ctid everywhere

98b139b

ensure subscription has ctid

6996d08

add wrong network error code

fc8477e

add wrong network id rpc test

ee8bbf1

clang-format

9b889e1

Merge branch 'develop' into patch-ctid

9575307

intelliot requested review from nbougalis, HowardHinnant, RichardAH and cindyyan317 October 2, 2023 22:48

intelliot mentioned this pull request Oct 2, 2023

feat: Add optional CTID field to Tx XRPLF/xrpl.js#2477

Merged

2 tasks

cindyyan317 mentioned this pull request Oct 3, 2023

Add Support for CTIDs (Concise Transaction Identifiers) XRPLF/clio#719

Open

intelliot added the API Change label Oct 3, 2023

intelliot assigned dangell7 Oct 4, 2023

Merge branch 'develop' into patch-ctid

67af70e

add tx tests

ffad0af

dangell7 marked this pull request as draft October 6, 2023 07:38

clang-format

8e1f5fb

intelliot added this to the 2.0 milestone Oct 13, 2023

intelliot mentioned this pull request Oct 17, 2023

[rpcWRONG_NETWORK does not have error info ] (Version: [2.0.0-b1]) #4735

Open

intelliot added the Clio Reviewed label Oct 17, 2023

Merge branch 'develop' into patch-ctid

12b8740

intelliot added the Perf Attn Needed Attention needed from RippleX Performance Team label Jan 9, 2024

intelliot modified the milestones: 2.1.0 (Mar 2024), 2.2.0 (Apr 2024) Feb 1, 2024

dangell7 added 2 commits February 1, 2024 18:06

Update Tx.cpp

9c16620

Merge branch 'develop' into patch-ctid

b90428a

dangell7 requested a review from scottschurr February 1, 2024 17:09

Update Transaction.cpp

65568cf

scottschurr approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'develop' into patch-ctid

a02a04d

dangell7 commented Feb 6, 2024

View reviewed changes

seelabs requested review from seelabs and removed request for nbougalis May 9, 2024 15:34

seelabs self-assigned this May 9, 2024

seelabs approved these changes May 13, 2024

View reviewed changes

Merge branch 'develop' into patch-ctid

47df4c2

intelliot modified the milestones: 2.2.0 (June 2024), 2.3.0 (August 2024) May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch CTID #4738

Patch CTID #4738

dangell7 commented Oct 2, 2023 •

edited by intelliot

Loading

cindyyan317 commented Oct 3, 2023

dangell7 commented Oct 3, 2023

cindyyan317 commented Oct 3, 2023

dangell7 commented Oct 4, 2023

dangell7 commented Oct 5, 2023

cindyyan317 commented Oct 5, 2023 •

edited

Loading

intelliot commented Oct 6, 2023

dangell7 commented Oct 6, 2023

dangell7 commented Oct 6, 2023

dangell7 commented Oct 31, 2023

intelliot commented Nov 2, 2023

intelliot commented Jan 9, 2024

intelliot commented Jan 17, 2024

intelliot commented Feb 1, 2024

codecov-commenter commented Feb 1, 2024 •

edited

Loading

intelliot commented Feb 2, 2024

scottschurr left a comment

dangell7 commented Feb 6, 2024

dangell7 Feb 6, 2024

dangell7 Feb 6, 2024

scottschurr Feb 6, 2024

dangell7 Feb 6, 2024

intelliot commented Feb 20, 2024

seelabs left a comment

dangell7 commented May 14, 2024

intelliot commented May 20, 2024

sophiax851 commented Jun 26, 2024

Patch CTID #4738

Are you sure you want to change the base?

Patch CTID #4738

Conversation

dangell7 commented Oct 2, 2023 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

cindyyan317 commented Oct 3, 2023

dangell7 commented Oct 3, 2023

cindyyan317 commented Oct 3, 2023

dangell7 commented Oct 4, 2023

dangell7 commented Oct 5, 2023

cindyyan317 commented Oct 5, 2023 • edited Loading

intelliot commented Oct 6, 2023

dangell7 commented Oct 6, 2023

dangell7 commented Oct 6, 2023

dangell7 commented Oct 31, 2023

intelliot commented Nov 2, 2023

intelliot commented Jan 9, 2024

intelliot commented Jan 17, 2024

intelliot commented Feb 1, 2024

codecov-commenter commented Feb 1, 2024 • edited Loading

Codecov Report

intelliot commented Feb 2, 2024

scottschurr left a comment

Choose a reason for hiding this comment

dangell7 commented Feb 6, 2024

dangell7 Feb 6, 2024

Choose a reason for hiding this comment

dangell7 Feb 6, 2024

Choose a reason for hiding this comment

scottschurr Feb 6, 2024

Choose a reason for hiding this comment

dangell7 Feb 6, 2024

Choose a reason for hiding this comment

intelliot commented Feb 20, 2024

seelabs left a comment

Choose a reason for hiding this comment

dangell7 commented May 14, 2024

intelliot commented May 20, 2024

sophiax851 commented Jun 26, 2024

dangell7 commented Oct 2, 2023 •

edited by intelliot

Loading

cindyyan317 commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Feb 1, 2024 •

edited

Loading