8000 Patch CTID by dangell7 · Pull Request #4738 · XRPLF/rippled · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Patch CTID #4738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 10, 2025
Merged

Patch CTID #4738

merged 3 commits into from
Apr 10, 2025

Conversation

dangell7
Copy link
Collaborator
@dangell7 dangell7 commented Oct 2, 2023 •
edited by intelliot
Loading

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

Context of Change

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)

@intelliot intelliot mentioned this pull request Oct 2, 2023
Merged
2 tasks
@cindyyan317
Copy link
Contributor

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:
Screenshot 2023-10-03 at 10 17 41

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

@dangell7
Copy link
Collaborator Author
dangell7 commented Oct 3, 2023

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: Screenshot 2023-10-03 at 10 17 41

"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
Copy link
Contributor

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: Screenshot 2023-10-03 at 10 17 41
"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?

@cindyyan317 cindyyan317 mentioned this pull request Oct 3, 2023
Closed
@dangell7
Copy link
Collaborator Author
dangell7 commented Oct 4, 2023

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: Screenshot 2023-10-03 at 10 17 41
"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
Copy link
Collaborator Author
dangell7 commented Oct 5, 2023

@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
Copy link
Contributor
cindyyan317 commented Oct 5, 2023 •
edited
Loading

@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.
Screenshot 2023-10-05 at 12 56 30

Screenshot 2023-10-05 at 12 55 44

@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
Copy link
Collaborator

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
Copy link
Collaborator Author
dangell7 commented Oct 6, 2023

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 dangell7 marked this pull request as draft October 6, 2023 07:38
@dangell7
Copy link
Collaborator Author
dangell7 commented Oct 6, 2023

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

@intelliot intelliot added this to the 2.0 milestone Oct 13, 2023
@intelliot intelliot mentioned this pull request Oct 17, 2023
Closed
@dangell7 dangell7 marked this pull request as ready for review October 19, 2023 09:41
@dangell7
Copy link
Collaborator Author

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

@intelliot intelliot requested review from mDuo13, ckeshava and scottschurr and removed request for HowardHinnant October 19, 2023 17:03
@intelliot intelliot assigned scottschurr and ckeshava and unassigned dangell7 Oct 19, 2023
seelabs
seelabs approved these changes May 13, 2024
View reviewed changes
Copy link
Collaborator
@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dangell7
Copy link
Collaborator Author

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

@intelliot
Copy link
Collaborator

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

@sophiax851
Copy link
Collaborator

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.

@sgramkumar
Copy link
Collaborator

@dangell7 running a test to use ctid in lowercase seems to be breaking the test

2025-02-05 10:33:51 (                    rippled.py:1769) INFO     {'method': 'tx', 'params': [{'ctid': 'c000012300000007', 'Fee': 1000}]}
2025-02-05 10:33:51 (                    rippled.py:1787) INFO       Response status: error
2025-02-05 10:33:51 (                  ctid_test.py:130 ) INFO     {'result': {'error': 'invalidParams', 'error_code': 31, 'error_message': 'Invalid parameters.', 'request': {'Fee': 1000, 'command': 'tx', 'ctid': 'c000012300000007'}, 'status': 'error'}}

Same works for upper case ctid

2025-02-05 11:05:43 (                    rippled.py:1769) INFO     {'method': 'tx', 'params': [{'ctid': 'C000001F00000007', 'Fee': 1000}]}
2025-02-05 11:05:43 (                    rippled.py:1787) INFO       Response status: success
2025-02-05 11:05:43 (                  ctid_test.py:130 ) INFO     {'result': {'Account': 'rPD1i8CacpThw8ysjWKhquX1R6BvfeXdjV', 'Amount': '1000', 'DeliverMax': '1000', 'Destination': 'rBEAWFjdLepAsmqSBNHdN7ph9sdPmKR8Ac', 'Fee': '1000', 'Flags': 2147483648, 'Sequence': 16, 'SigningPubKey': '02538BA5CC8BF439B8411C21B100283008AED149E27BE46FA0D683288FA5CA8B06', 'TransactionType': 'Payment', 'TxnSignature': '30450221009FAB43B32C16FEF2FB362F3063A7C1868B9636EE3329C2EB444E65071761A1F802205B44603505F0DCC00E03A2E426FAAB673FA320F2A2A19716AC0F3E63A948D4C1', 'ctid': 'C000001F00000007', 'date': 792097568, 'hash': '659A69CC7F299156EAAB382D5A5C77047F1D0D57AB3C9541911D39927DBF7ECD', 'inLedger': 31, 'ledger_index': 31, 'meta': {'AffectedNodes': [{'ModifiedNode': {'FinalFields': {'Account': 'rPD1i8CacpThw8ysjWKhquX1R6BvfeXdjV', 'Balance': '1999996000', 'Flags': 0, 'OwnerCount': 0, 'Sequence': 17}, 'LedgerEntryType': 'AccountRoot', 'LedgerIndex': '7240AF71D70203DBA8811044DD7668F253784B26E67E1ACCA21B5C55FAA4F900', 'PreviousFields': {'Balance': '1999998000', 'Sequence': 16}, 'PreviousTxnID': 'FFC2142D82425257E83AAD939039DD9E551D6523538F47FD7CE623AB294F9F94', 'PreviousTxnLgrSeq': 27}}, {'ModifiedNode': {'FinalFields': {'Account': 'rBEAWFjdLepAsmqSBNHdN7ph9sdPmKR8Ac', 'Balance': '2000002000', 'Flags': 0, 'OwnerCount': 0, 'Sequence': 22}, 'LedgerEntryType': 'AccountRoot', 'LedgerIndex': 'E2FAC88A9631B61F75DBA34FA558E0D384D740FA660A5C223F8999F33520687E', 'PreviousFields': {'Balance': '2000001000'}, 'PreviousTxnID': 'FFC2142D82425257E83AAD939039DD9E551D6523538F47FD7CE623AB294F9F94', 'PreviousTxnLgrSeq': 27}}], 'TransactionIndex': 0, 'TransactionResult': 'tesSUCCESS', 'delivered_amount': '1000'}, 'status': 'success', 'validated': True}}

Also, this PR needs to be rebased. Still hitting server blocked

2025-Feb-05 18:02:39.486379 UTC LedgerMaster:ERR One or more unsupported amendments activated: server blocked.

@bthomee bthomee removed this from the 2.4.0 (Q1 2025) milestone Feb 11, 2025
@ximinez
Copy link
Collaborator
ximinez commented Mar 18, 2025

@dangell7 @Transia-RnD This has two valid reviews, and the only changes after were merges from develop. Other than needing develop merged in again, conflicts resolved, and a good commit message is this ready to merge?

@dangell7
Copy link
Collaborator Author

@dangell7 @Transia-RnD This has two valid reviews, and the only changes after were merges from develop. Other than needing develop merged in again, conflicts resolved, and a good commit message is this ready to merge?

Yes I believe so. However the reason it was not merged is because it requires a perf signoff by @sophiax851. Hopefully that can continue now.

@dangell7
Copy link
Collaborator Author

Commit Message:

fix: CTID improvements

Update RPC handlers and transaction processing logic to correctly
generate and include Concise Transaction Identifiers (CTIDs) in
responses when appropriate. Add a new RPC error code rpcWRONG_NETWORK
to handle cases where a CTID is submitted to a node running on a
different network. Improve CTID validation logic and expand unit
tests to cover network ID boundary conditions and error scenarios.

@sophiax851
Copy link
Collaborator

I'm traveling today, I'll try to run a load and see if there's any regression comparing with a same load on 2.4.0.

@sophiax851
Copy link
Collaborator

Hi @dangell7, Is this branch based off the 2.4.0? I've done some testing with the following Load Scenario:
A TX load of 500 TPS combined with a total of ~150 TPS of mixed transaction load injected on the 4 client nodes of a private XRPL network consisting of 5 validators and 4 CH nodes.
A set of comparison tests were run on the same network running rippled 2.4.0 vs. rippled built from this PR. The results are showing about 22% of latency improvement with the TX RPC calls and a 13% of improvement with the Submit RPC calls. There's also a slight improvement with Ledger Consensus latency.
I'm very curious about the source of the improvements if this branch was indeed built off rippled_2.4.0. I will run another test on 2.4.0 just to verify the results are consistent.
cc: @ximinez

@dangell7
Copy link
Collaborator Author

Hi @dangell7, Is this branch based off the 2.4.0? I've done some testing with the following Load Scenario: A TX load of 500 TPS combined with a total of ~150 TPS of mixed transaction load injected on the 4 client nodes of a private XRPL network consisting of 5 validators and 4 CH nodes. A set of comparison tests were run on the same network running rippled 2.4.0 vs. rippled built from this PR. The results are showing about 22% of latency improvement with the TX RPC calls and a 13% of improvement with the Submit RPC calls. There's also a slight improvement with Ledger Consensus latency. I'm very curious about the source of the improvements if this branch was indeed built off rippled_2.4.0. I will run another test on 2.4.0 just to verify the results are consistent. cc: @ximinez

Well thats very interesting. I just updated the branch just in case but I'm pretty sure I rebased off 2.4.0 last week.

@sophiax851
Copy link
Collaborator
sophiax851 commented Mar 26, 2025 •
edited
Loading

I think your previous branch was based off 2.4.0:
"ubuntu@sophia-vl2:/opt/ripple/bin$ ./rippled_patchCTID --version
rippled version 2.4.0
Git commit hash: 08a3775
Git build branch: patch-ctid"
I reran the same test on 2.4.0 again, the difference was almost all gone, then I took a look of the graphs, looks like the previous test on 2.4.0 had some network issue with the environment at the end of the test, which skewed the results, see below:
Screenshot 2025-03-26 at 1 48 22 PM

I think we are all good, I'd sign off on this PR. Thanks for your work!!

@mvadari
Copy link
Collaborator
mvadari commented Apr 9, 2025

@bthomee this looks good to go

@bthomee bthomee requested a review from godexsoft April 10, 2025 07:47
godexsoft
godexsoft approved these changes Apr 10, 2025
View reviewed changes
Copy link
Collaborator
@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from Clio side. Tx handler's logic looks fine.

@bthomee
Copy link
Collaborator
bthomee commented Apr 10, 2025

@dangell7 can you please update the branch? I'll merge it right afterwards.

@bthomee bthomee enabled auto-merge (squash) April 10, 2025 11:55
@bthomee bthomee merged commit c4308b2 into XRPLF:develop Apr 10, 2025
24 checks passed
@github-project-automation github-project-automation bot moved this from 👀 Needs review to ✅ Merged in Core Ledger Apr 10, 2025
@kennyzlei kennyzlei mentioned this pull request Apr 23, 2025
Closed
Tapanito pushed a commit that referenced this pull request Apr 24, 2025
@dangell7 @Tapanito
fix: Adds CTID to RPC tx and updates error (#4738)
d08f3ac 
This change fixes a number of issues involved with CTID:
* CTID is not present on all RPC tx transactions.
* rpcWRONG_NETWORK is missing in the ErrorCodes.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottschurr scottschurr scottschurr approved these changes

@godexsoft godexsoft godexsoft approved these changes

@seelabs seelabs seelabs approved these changes

@RichardAH RichardAH Awaiting requested review from RichardAH

@cindyyan317 cindyyan317 Awaiting requested review from cindyyan317

@mDuo13 mDuo13 Awaiting requested review from mDuo13

@ckeshava ckeshava Awaiting requested review from ckeshava

Assignees

@seelabs seelabs

@dangell7 dangell7

Labels
API Change Clio Reviewed Perf Attn Needed Attention needed from RippleX Performance Team Testable
Projects
Core Ledger
Status: ✅ Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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