Skip to content
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.

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

feat: support Concise Transaction Identifier (CTID) (XLS-37) #4418

Merged
merged 30 commits into from
Aug 18, 2023

Conversation

RichardAH
Copy link
Collaborator

@RichardAH RichardAH commented Feb 13, 2023

High Level Overview of Change

RPC support for CTID

CTID is a network-aware short alternative to using transaction hashes that is based on the ledger sequence, transaction index and network ID of a validated transaction in a closed ledger.

  • This PR only adds CTID in the tx response

This implements XLS-37

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 Updates
  • Release

@intelliot
Copy link
Collaborator

@RichardAH says this is ready to be reviewed. At this time, he does not plan to do more for this. However, he will check with Denis to see if he has done some work on it.

@dangell7
Copy link
Collaborator

@intelliot There is another commit that I have here. Basically if you add the network config, all the env functions wont work. Like fund, pay etc. So this is basically a workaround autofill.

This PR will likely fail because this is not included. I can cherry pick this commit in if you'd like or I can rewrite the tests to manually inject the networkID

@intelliot
Copy link
Collaborator

That's up to you, but I would recommend proposing a PR that, as far as you know, would be ready to merge into develop (if it passes code review and testing). That could look like additional commits/merges into this PR (RichardAH:ctim), or closing this PR and opening a new one from a fork/branch that you create. (The latter is worth considering since this PR's author is @RichardAH rather than @dangell7; but in the end, what really matters is that there is a PR that is complete and ready to go.)

@dangell7
Copy link
Collaborator

@intelliot This is now ready for review.

@RichardAH
Copy link
Collaborator Author

This should be merged prior to any parallel / side chains being launched

@intelliot intelliot changed the title Add Improved Concise Transaction Identifier support Add Improved Concise Transaction Identifier support (CTID) May 26, 2023
@RichardAH RichardAH changed the title Add Improved Concise Transaction Identifier support (CTID) Ensure USERS can FIND their TXN on SIDECHAINS-- Concise Transaction Identifier (CTID) May 26, 2023
@intelliot intelliot requested a review from seelabs May 26, 2023 18:56
src/ripple/rpc/handlers/Tx.cpp Outdated Show resolved Hide resolved
Builds/CMake/RippledCore.cmake Show resolved Hide resolved
src/ripple/app/tx/impl/Transactor.cpp Outdated Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerMaster.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Tx.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Tx.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Tx.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/Tx.cpp Outdated Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerMaster.cpp Show resolved Hide resolved
fix bad seperator

clang-format
@RichardAH
Copy link
Collaborator Author

I believe I was initially brought in to review why tests were failing. That led to my diagnosis. I further noticed that Nik's last comment seemed to be unresolved, and so I reviewed that part of the code too. I can confirm now that the tests pass, and that Nik's objection is resolved, and so I add my approval, but with the disclosure that I have not reviewed all of the code here, leaving that up to the ones who came before me.

Thanks john

@intelliot
Copy link
Collaborator

@RichardAH from your perspective, is this PR now ready to merge, or would you prefer to have @nbougalis take a look first?

Also, it is suggested that this PR be squashed into 1 commit when it is merged, with the following commit message. If this message isn't ideal, please feel free to propose an alternative.

feat: support Concise Transaction Identifier (CTID) (XLS-37) (#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  https://github.com/XRPLF/XRPL-Standards/pull/111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

Note: I updated XLS037 to XLS-37 to match the typical formatting used when other XLS specs have been referenced.

@intelliot intelliot changed the title feat: support Concise Transaction Identifier (CTID) (XLS037) feat: support Concise Transaction Identifier (CTID) (XLS-37) Jul 20, 2023
@RichardAH
Copy link
Collaborator Author

I will do a final review shortly. It’s been a long time since I wrote this code and I need to squash and review to see what ended up in it after other people’s changes

@RichardAH
Copy link
Collaborator Author

LGTM

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 26, 2023
@intelliot
Copy link
Collaborator

intelliot commented Jul 26, 2023

LGTM

I take this to mean that this PR is ready to merge. @manojsdoshi - please consider this PR for the next release, and squash with the commit message in #4418 (comment)

@manojsdoshi manojsdoshi merged commit 5530a0b into XRPLF:develop Aug 18, 2023
15 checks passed
intelliot added a commit that referenced this pull request Aug 18, 2023
The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Elliot Lee <github.public@intelliot.com>
Co-authored-by: Denis Angell <dangell@transia.co>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
(XLS-37) (XRPLF#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Elliot Lee <github.public@intelliot.com>
Co-authored-by: Denis Angell <dangell@transia.co>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
(XLS-37) (XRPLF#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Elliot Lee <github.public@intelliot.com>
Co-authored-by: Denis Angell <dangell@transia.co>
Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
(XLS-37) (XRPLF#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
(XLS-37) (XRPLF#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>
@manojsdoshi manojsdoshi mentioned this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
(XLS-37) (XRPLF#4418)

The XLS-37 CTID (Concise Transaction ID) is a network-aware tx
identifier which provides a way to efficiently locate a specific
transaction without relying on transaction hashes. A CTID encodes the
sequence number of the ledger that includes the tx, the transaction's
index in that ledger, and the network ID. With the CTID, users can
identify transactions and confirm their results. This applies even for
transactions on sidechains, which may be difficult to find with only a
transaction hash.

Additionally, CTIDs require less storage space than transaction hashes,
which can be beneficial for databases storing millions of transactions.

The XLS-37 specification can be found at:
  XRPLF/XRPL-Standards#111

Add support for running a node on a different network. There is a new
error code, `rpcWRONG_NETWORK`, returned when the requested CTID's
network ID does not match the node's network ID. The error message looks
like:

  Wrong network. You should submit this request to a node running on
  NetworkID: <net_id>

* Add RPC support for the CTID format
  * tx - you can specify "ctid", which is the CTID (16 hex digits, in a
    string, always starting with "C")
  * When retrieving a tx, the "ctid" may be returned
* Add support for encoding, decoding, and validating CTIDs
* Add tests

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Denis Angell <dangell@transia.co>

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
)

* add CTIM to tx rpc

---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Elliot Lee <github.public@intelliot.com>
Co-authored-by: Denis Angell <dangell@transia.co>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
)

* add CTIM to tx rpc


---------

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
Co-authored-by: Elliot Lee <github.public@intelliot.com>
Co-authored-by: Denis Angell <dangell@transia.co>
@@ -47,8 +47,8 @@ enum error_code_i {
rpcJSON_RPC = 2,
rpcFORBIDDEN = 3,

rpcWRONG_NETWORK = 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to add "rpcWRONG_NETWORK" in unorderedErrorInfos ( ErrorCodes.cpp )


uint64_t ctidValue =
((0xC000'0000ULL + static_cast<uint64_t>(ledger_seq)) << 32) +
(static_cast<uint64_t>(txn_index) << 16) + network_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Clio Reviewed Feature Request Used to indicate requests to add new features Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Testable Will Need Documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet