TicketSequence with non-zero Sequence is an error:

Before this change any non-zero Sequence field was handled as
a non-ticketed transaction, even if a TicketSequence was
present.  We learned that this could lead to user confusion.
So the rules are tightened up.

Now if any transaction contains both a non-zero Sequence
field and a TicketSequence field then that transaction
returns a temSEQ_AND_TICKET error code.

The (deprecated) "sign" and "submit" RPC commands are tuned
up so they auto-insert a Sequence field of zero if they see
a TicketSequence in the transaction.

No amendment is needed because this change is going into
the first release that supports the TicketBatch amendment.

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())
     {

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,
 };
 
 //------------------------------------------------------------------------------

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."),

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))

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();
     }
 };