diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index bc7a98308..5cfaabca8 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -57,7 +57,7 @@ preflight1(PreflightContext const& ctx) { // This is inappropriate in preflight0, because only Change transactions // skip this function, and those do not allow an sfTicketSequence field. - if (ctx.tx.getSeqProxy().isTicket() && + if (ctx.tx.isFieldPresent(sfTicketSequence) && !ctx.rules.enabled(featureTicketBatch)) { return temMALFORMED; @@ -254,18 +254,29 @@ Transactor::checkSeqProxy( SeqProxy const t_seqProx = tx.getSeqProxy(); SeqProxy const a_seq = SeqProxy::sequence((*sle)[sfSequence]); - if (t_seqProx.isSeq() && t_seqProx != a_seq) + if (t_seqProx.isSeq()) { - if (a_seq < t_seqProx) + if (tx.isFieldPresent(sfTicketSequence) && + view.rules().enabled(featureTicketBatch)) { - JLOG(j.trace()) << "applyTransaction: has future sequence number " - << "a_seq=" << a_seq << " t_seq=" << t_seqProx; - return terPRE_SEQ; + JLOG(j.trace()) << "applyTransaction: has both a TicketSequence " + "and a non-zero Sequence number"; + return temSEQ_AND_TICKET; + } + if (t_seqProx != a_seq) + { + if (a_seq < t_seqProx) + { + JLOG(j.trace()) + << "applyTransaction: has future sequence number " + << "a_seq=" << a_seq << " t_seq=" << t_seqProx; + return terPRE_SEQ; + } + // It's an already-used sequence number. + JLOG(j.trace()) << "applyTransaction: has past sequence number " + << "a_seq=" << a_seq << " t_seq=" << t_seqProx; + return tefPAST_SEQ; } - // It's an already-used sequence number. - JLOG(j.trace()) << "applyTransaction: has past sequence number " - << "a_seq=" << a_seq << " t_seq=" << t_seqProx; - return tefPAST_SEQ; } else if (t_seqProx.isTicket()) { diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 41f271ed7..7077a1ac0 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -115,9 +115,10 @@ enum TEMcodes : TERUnderlyingType { temCANNOT_PREAUTH_SELF, temINVALID_COUNT, - // An intermediate result used internally, should never be returned. - temUNCERTAIN, - temUNKNOWN, + temUNCERTAIN, // An internal intermediate result; should never be returned. + temUNKNOWN, // An internal intermediate result; should never be returned. + + temSEQ_AND_TICKET, }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index b8dc0abb7..b0592d248 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -152,6 +152,7 @@ transResults() MAKE_ERROR(temINVALID_ACCOUNT_ID, "Malformed: A field contains an invalid account ID."), MAKE_ERROR(temCANNOT_PREAUTH_SELF, "Malformed: An account may not preauthorize itself."), MAKE_ERROR(temINVALID_COUNT, "Malformed: Count field outside valid range."), + MAKE_ERROR(temSEQ_AND_TICKET, "Transaction contains a TicketSequence and a non-zero Sequence."), MAKE_ERROR(terRETRY, "Retry transaction."), MAKE_ERROR(terFUNDS_SPENT, "DEPRECATED."), diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 10eadd671..df443d816 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -441,7 +441,9 @@ transactionPreProcessImpl( { if (!tx_json.isMember(jss::Sequence)) { - if (!sle) + bool const hasTicketSeq = + tx_json.isMember(sfTicketSequence.jsonName); + if (!hasTicketSeq && !sle) { JLOG(j.debug()) << "transactionSign: Failed to find source account " @@ -449,7 +451,8 @@ transactionPreProcessImpl( return rpcError(rpcSRC_ACT_NOT_FOUND); } - tx_json[jss::Sequence] = app.getTxQ().nextQueuableSeq(sle).value(); + tx_json[jss::Sequence] = + hasTicketSeq ? 0 : app.getTxQ().nextQueuableSeq(sle).value(); } if (!tx_json.isMember(jss::Flags)) diff --git a/src/test/app/Ticket_test.cpp b/src/test/app/Ticket_test.cpp index 331ed68be..c6907fb3b 100644 --- a/src/test/app/Ticket_test.cpp +++ b/src/test/app/Ticket_test.cpp @@ -390,6 +390,10 @@ class Ticket_test : public beast::unit_test::suite env.require(owners(env.master, 0), tickets(env.master, 0)); env(noop(env.master), ticket::use(1), ter(temMALFORMED)); + env(noop(env.master), + ticket::use(1), + seq(env.seq(env.master)), + ter(temMALFORMED)); // Close enough ledgers that the previous transactions are no // longer retried. @@ -826,6 +830,159 @@ class Ticket_test : public beast::unit_test::suite checkTxFromDB(txHash_8, 5, 0, 7, ttACCOUNT_SET); } + void + testSignWithTicketSequence() + { + // The sign and the submit RPC commands automatically fill in the + // Sequence field of a transaction if none is provided. If a + // TicketSequence is provided in the transaction, then the + // auto-filled Sequence should be zero. + testcase("Sign with TicketSequence"); + + using namespace test::jtx; + Env env{*this}; + Account alice{"alice"}; + + env.fund(XRP(10000), alice); + env.close(); + + // Successfully create tickets (using a sequence) + std::uint32_t const ticketSeq = env.seq(alice) + 1; + env(ticket::create(alice, 2)); + checkTicketCreateMeta(env); + env.close(); + env.require(owners(alice, 2), tickets(alice, 2)); + BEAST_EXPECT(ticketSeq + 2 == env.seq(alice)); + + { + // Test that the "sign" RPC command fills in a "Sequence": 0 field + // if none is provided. + + // Create a noop transaction using a TicketSequence but don't fill + // in the Sequence field. + Json::Value tx = Json::objectValue; + tx[jss::tx_json] = noop(alice); + tx[jss::tx_json][sfTicketSequence.jsonName] = ticketSeq; + tx[jss::secret] = toBase58(generateSeed("alice")); + + // Verify that there is no "Sequence" field. + BEAST_EXPECT(!tx[jss::tx_json].isMember(sfSequence.jsonName)); + + // Call the "sign" RPC command and see the "Sequence": 0 field + // filled in. + Json::Value jr = env.rpc("json", "sign", to_string(tx)); + + // Verify that "sign" inserted a "Sequence": 0 field. + if (BEAST_EXPECT(jr[jss::result][jss::tx_json].isMember( + sfSequence.jsonName))) + { + BEAST_EXPECT( + jr[jss::result][jss::tx_json][sfSequence.jsonName] == 0); + } + + // "sign" should not have consumed any of alice's tickets. + env.close(); + env.require(owners(alice, 2), tickets(alice, 2)); + + // "submit" the signed blob and see one of alice's tickets consumed. + env.rpc("submit", jr[jss::result][jss::tx_blob].asString()); + env.close(); + env.require(owners(alice, 1), tickets(alice, 1)); + } + { + // Test that the "submit" RPC command fills in a "Sequence": 0 + // field if none is provided. + + // Create a noop transaction using a TicketSequence but don't fill + // in the Sequence field. + Json::Value tx = Json::objectValue; + tx[jss::tx_json] = noop(alice); + tx[jss::tx_json][sfTicketSequence.jsonName] = ticketSeq + 1; + tx[jss::secret] = toBase58(generateSeed("alice")); + + // Verify that there is no "Sequence" field. + BEAST_EXPECT(!tx[jss::tx_json].isMember(sfSequence.jsonName)); + + // Call the "submit" RPC command and see the "Sequence": 0 field + // filled in. + Json::Value jr = env.rpc("json", "submit", to_string(tx)); + + // Verify that "submit" inserted a "Sequence": 0 field. + if (BEAST_EXPECT(jr[jss::result][jss::tx_json].isMember( + sfSequence.jsonName))) + { + BEAST_EXPECT( + jr[jss::result][jss::tx_json][sfSequence.jsonName] == 0); + } + + // "submit" should have consumed the last of alice's tickets. + env.close(); + env.require(owners(alice, 0), tickets(alice, 0)); + } + } + + void + testFixBothSeqAndTicket() + { + // It is an error if a transaction contains a non-zero Sequence field + // and a TicketSequence field. Verify that the error is detected. + testcase("Fix both Seq and Ticket"); + + // Try the test without featureTicketBatch enabled. + using namespace test::jtx; + { + Env env{*this, supported_amendments() - featureTicketBatch}; + Account alice{"alice"}; + + env.fund(XRP(10000), alice); + env.close(); + + // Fail to create a ticket. + std::uint32_t const ticketSeq = env.seq(alice) + 1; + env(ticket::create(alice, 1), ter(temDISABLED)); + env.close(); + env.require(owners(alice, 0), tickets(alice, 0)); + BEAST_EXPECT(ticketSeq == env.seq(alice) + 1); + + // Create a transaction that includes both a ticket and a non-zero + // sequence number. Since a ticket is used and tickets are not yet + // enabled the transaction should be malformed. + env(noop(alice), + ticket::use(ticketSeq), + seq(env.seq(alice)), + ter(temMALFORMED)); + env.close(); + } + // Try the test with featureTicketBatch enabled. + { + Env env{*this, supported_amendments()}; + Account alice{"alice"}; + + env.fund(XRP(10000), alice); + env.close(); + + // Create a ticket. + std::uint32_t const ticketSeq = env.seq(alice) + 1; + env(ticket::create(alice, 1)); + env.close(); + env.require(owners(alice, 1), tickets(alice, 1)); + BEAST_EXPECT(ticketSeq + 1 == env.seq(alice)); + + // Create a transaction that includes both a ticket and a non-zero + // sequence number. The transaction fails with temSEQ_AND_TICKET. + env(noop(alice), + ticket::use(ticketSeq), + seq(env.seq(alice)), + ter(temSEQ_AND_TICKET)); + env.close(); + + // Verify that the transaction failed by looking at alice's + // sequence number and tickets. + env.require(owners(alice, 1), tickets(alice, 1)); + BEAST_EXPECT(ticketSeq + 1 == env.seq(alice)); + } + } + public: void run() override @@ -836,6 +993,8 @@ public: testTicketInsufficientReserve(); testUsingTickets(); testTransactionDatabaseWithTickets(); + testSignWithTicketSequence(); + testFixBothSeqAndTicket(); } };