diff --git a/src/ripple/app/ledger/TransactionMaster.h b/src/ripple/app/ledger/TransactionMaster.h index abac6da82ce..2af157467db 100644 --- a/src/ripple/app/ledger/TransactionMaster.h +++ b/src/ripple/app/ledger/TransactionMaster.h @@ -78,7 +78,7 @@ class TransactionMaster uint256 const& hash, std::uint32_t ledger, std::optional tseq, - std::optional netID); + std::optional netID); void canonicalize(std::shared_ptr* pTransaction); diff --git a/src/ripple/app/ledger/impl/TransactionMaster.cpp b/src/ripple/app/ledger/impl/TransactionMaster.cpp index 0301d95dcab..4eec31eefe7 100644 --- a/src/ripple/app/ledger/impl/TransactionMaster.cpp +++ b/src/ripple/app/ledger/impl/TransactionMaster.cpp @@ -41,7 +41,7 @@ TransactionMaster::inLedger( uint256 const& hash, std::uint32_t ledger, std::optional tseq, - std::optional netID) + std::optional netID) { auto txn = mCache.fetch(hash); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 3669f082fa1..5d4d9fd6ed1 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -3107,14 +3107,10 @@ NetworkOPsImp::transJson( if (transaction.isFieldPresent(sfNetworkID)) netID = transaction.getFieldU32(sfNetworkID); - if (txnSeq <= 0xFFFFU && netID < 0xFFFFU && - ledger->info().seq < 0xFFFFFFFUL) - { - if (std::optional ctid = - RPC::encodeCTID(ledger->info().seq, txnSeq, netID); - ctid) - jvObj[jss::ctid] = *ctid; - } + if (std::optional ctid = + RPC::encodeCTID(ledger->info().seq, txnSeq, netID); + ctid) + jvObj[jss::ctid] = *ctid; } if (validated) diff --git a/src/ripple/app/misc/Transaction.h b/src/ripple/app/misc/Transaction.h index 2dcb0d6796f..5f36050aae8 100644 --- a/src/ripple/app/misc/Transaction.h +++ b/src/ripple/app/misc/Transaction.h @@ -131,7 +131,7 @@ class Transaction : public std::enable_shared_from_this, TransStatus status, std::uint32_t ledgerSeq, std::optional transactionSeq = std::nullopt, - std::optional networkID = std::nullopt); + std::optional networkID = std::nullopt); void setStatus(TransStatus status) @@ -392,7 +392,7 @@ class Transaction : public std::enable_shared_from_this, LedgerIndex mInLedger = 0; std::optional mTxnSeq; - std::optional mNetworkID; + std::optional mNetworkID; TransStatus mStatus = INVALID; TER mResult = temUNCERTAIN; diff --git a/src/ripple/app/misc/impl/Transaction.cpp b/src/ripple/app/misc/impl/Transaction.cpp index 7692f04273f..82aca01c888 100644 --- a/src/ripple/app/misc/impl/Transaction.cpp +++ b/src/ripple/app/misc/impl/Transaction.cpp @@ -64,7 +64,7 @@ Transaction::setStatus( TransStatus ts, std::uint32_t lseq, std::optional tseq, - std::optional netID) + std::optional netID) { mStatus = ts; mInLedger = lseq; @@ -196,8 +196,7 @@ Transaction::getJson(JsonOptions options, bool binary) const if (mTransaction->isFieldPresent(sfNetworkID)) netID = mTransaction->getFieldU32(sfNetworkID); - if (mTxnSeq && netID && *mTxnSeq <= 0xFFFFU && *netID < 0xFFFFU && - mInLedger < 0xFFFFFFFUL) + if (mTxnSeq && netID) { std::optional ctid = RPC::encodeCTID(mInLedger, *mTxnSeq, *netID); diff --git a/src/ripple/rpc/CTID.h b/src/ripple/rpc/CTID.h index 8f6c64bc028..3bfa6165076 100644 --- a/src/ripple/rpc/CTID.h +++ b/src/ripple/rpc/CTID.h @@ -30,18 +30,24 @@ namespace ripple { namespace RPC { +// CTID stands for Compact Transaction ID. +// +// The CTID comes from XLS-15d: Concise Transaction Identifier #34 +// +// https://github.com/XRPLF/XRPL-Standards/discussions/34 +// +// The Compact Transaction ID provides a way to identify a transaction +// that includes which network the transaction was submitted to. + inline std::optional -encodeCTID( - uint32_t ledger_seq, - uint16_t txn_index, - uint16_t network_id) noexcept +encodeCTID(uint32_t ledgerSeq, uint32_t txnIndex, uint32_t networkID) noexcept { - if (ledger_seq > 0x0FFF'FFFF) + if (ledgerSeq > 0x0FFF'FFFF || txnIndex > 0xFFFF || networkID > 0xFFFF) return {}; uint64_t ctidValue = - ((0xC000'0000ULL + static_cast(ledger_seq)) << 32) + - (static_cast(txn_index) << 16) + network_id; + ((0xC000'0000ULL + static_cast(ledgerSeq)) << 32) + + (static_cast(txnIndex) << 16) + networkID; std::stringstream buffer; buffer << std::hex << std::uppercase << std::setfill('0') << std::setw(16) diff --git a/src/test/rpc/Transaction_test.cpp b/src/test/rpc/Transaction_test.cpp index 12a5c0ba35e..dac76692f7b 100644 --- a/src/test/rpc/Transaction_test.cpp +++ b/src/test/rpc/Transaction_test.cpp @@ -564,20 +564,10 @@ class Transaction_test : public beast::unit_test::suite BEAST_EXPECT(!RPC::encodeCTID(0x1000'0000UL, 0xFFFFU, 0xFFFFU)); // Test case 3: txn_index greater than 0xFFFF - // this test case is impossible in c++ due to the type, left in for - // completeness - auto const expected3 = std::optional("CFFFFFFF0000FFFF"); - BEAST_EXPECT( - RPC::encodeCTID(0x0FFF'FFFF, (uint16_t)0x10000, 0xFFFF) == - expected3); + BEAST_EXPECT(!RPC::encodeCTID(0x0FFF'FFFF, 0x1'0000, 0xFFFF)); // Test case 4: network_id greater than 0xFFFF - // this test case is impossible in c++ due to the type, left in for - // completeness - auto const expected4 = std::optional("CFFFFFFFFFFF0000"); - BEAST_EXPECT( - RPC::encodeCTID(0x0FFF'FFFFUL, 0xFFFFU, (uint16_t)0x1000'0U) == - expected4); + BEAST_EXPECT(!RPC::encodeCTID(0x0FFF'FFFFUL, 0xFFFFU, 0x1'0000U)); // Test case 5: Valid input values auto const expected51 = @@ -645,10 +635,11 @@ class Transaction_test : public beast::unit_test::suite using namespace test::jtx; - // test that the ctid AND the hash are in the response + // Use a Compact Transaction Identifier to request a transaction. + for (uint32_t netID : {11111, 65535, 65536}) { - Env env{*this, makeNetworkConfig(11111)}; - uint32_t netID = env.app().config().NETWORK_ID; + Env env{*this, makeNetworkConfig(netID)}; + BEAST_EXPECT(netID == env.app().config().NETWORK_ID); auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -658,39 +649,57 @@ class Transaction_test : public beast::unit_test::suite env(pay(alice, bob, XRP(10))); env.close(); - auto const ctid = *RPC::encodeCTID(startLegSeq, 0, netID); + auto const ctid = RPC::encodeCTID(startLegSeq, 0, netID); + if (netID > 0xFFFF) + { + // Compact transaction IDs do not support a network ID > 0xFFFF. + BEAST_EXPECT(ctid == std::nullopt); + continue; + } + Json::Value jsonTx; jsonTx[jss::binary] = false; - jsonTx[jss::ctid] = ctid; + jsonTx[jss::ctid] = *ctid; jsonTx[jss::id] = 1; auto const jrr = env.rpc("json", "tx", to_string(jsonTx))[jss::result]; BEAST_EXPECT(jrr[jss::ctid] == ctid); - BEAST_EXPECT(jrr[jss::hash]); + BEAST_EXPECT(jrr.isMember(jss::hash)); } - // test that if the network is 65535 the ctid is not in the response + // Using a hash to request the transaction, test the network ID + // boundary where the CTID is (not) in the response. + for (uint32_t netID : {2, 1024, 65535, 65536}) { - Env env{*this, makeNetworkConfig(65535)}; - uint32_t netID = env.app().config().NETWORK_ID; + Env env{*this, makeNetworkConfig(netID)}; + BEAST_EXPECT(netID == env.app().config().NETWORK_ID); auto const alice = Account("alice"); auto const bob = Account("bob"); - auto const startLegSeq = env.current()->info().seq; env.fund(XRP(10000), alice, bob); env(pay(alice, bob, XRP(10))); env.close(); - auto const ctid = *RPC::encodeCTID(startLegSeq, 0, netID); - Json::Value jsonTx; - jsonTx[jss::binary] = false; - jsonTx[jss::ctid] = ctid; - jsonTx[jss::id] = 1; + auto const ledgerSeq = env.current()->info().seq; + + env(noop(alice), ter(tesSUCCESS)); + env.close(); + + Json::Value params; + params[jss::id] = 1; + auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash]; + params[jss::transaction] = hash; auto const jrr = - env.rpc("json", "tx", to_string(jsonTx))[jss::result]; - BEAST_EXPECT(!jrr[jss::ctid]); - BEAST_EXPECT(jrr[jss::hash]); + env.rpc("json", "tx", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::hash] == hash); + + BEAST_EXPECT(jrr.isMember(jss::ctid) == (netID <= 0xFFFF)); + if (jrr.isMember(jss::ctid)) + { + auto const ctid = RPC::encodeCTID(ledgerSeq, 0, netID); + BEAST_EXPECT(jrr[jss::ctid] == *ctid); + } } // test the wrong network ID was submitted @@ -720,62 +729,6 @@ class Transaction_test : public beast::unit_test::suite "Wrong network. You should submit this request to a node " "running on NetworkID: 21338"); } - - // test the ctid is returned when network id < 1024 - { - Env env{*this, makeNetworkConfig(2)}; - uint32_t netID = env.app().config().NETWORK_ID; - - auto const alice = Account("alice"); - auto const bob = Account("bob"); - - env.fund(XRP(10000), alice, bob); - env(pay(alice, bob, XRP(10))); - env.close(); - - auto const ledgerSeq = env.current()->info().seq; - - env(noop(alice), ter(tesSUCCESS)); - env.close(); - - Json::Value params; - params[jss::id] = 1; - auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash]; - params[jss::transaction] = hash; - auto const jrr = - env.rpc("json", "tx", to_string(params))[jss::result]; - auto const ctid = *RPC::encodeCTID(ledgerSeq, 0, netID); - BEAST_EXPECT(jrr[jss::ctid] == ctid); - BEAST_EXPECT(jrr[jss::hash] == hash); - } - - // test the ctid is returned when network id == 1024 - { - Env env{*this, makeNetworkConfig(1024)}; - uint32_t netID = env.app().config().NETWORK_ID; - - auto const alice = Account("alice"); - auto const bob = Account("bob"); - - env.fund(XRP(10000), alice, bob); - env(pay(alice, bob, XRP(10))); - env.close(); - - auto const ledgerSeq = env.current()->info().seq; - - env(noop(alice), ter(tesSUCCESS)); - env.close(); - - Json::Value params; - params[jss::id] = 1; - auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash]; - params[jss::transaction] = hash; - auto const jrr = - env.rpc("json", "tx", to_string(params))[jss::result]; - auto const ctid = *RPC::encodeCTID(ledgerSeq, 0, netID); - BEAST_EXPECT(jrr[jss::ctid] == ctid); - BEAST_EXPECT(jrr[jss::hash] == hash); - } } public: