Always check the sequence when adding to the transaction queue:

* If multiple transactions are queued for the account, change the
  account's sequence number in a temporary view before processing the
  transaction.
* Adds a new "at()" interface to STObject which is identical to the
  operator[], but easier to write and read when dealing with ptrs.
* Split the TxQ tests into two suites to speed up parallel run times.

src/ripple/app/misc/impl/TxQ.cpp

@@ -1120,6 +1120,14 @@ TxQ::apply(
                 std::min(balance - std::min(balance, reserve), potentialSpend);
             assert(potentialTotalSpend > XRPAmount{0});
             sleBump->setFieldAmount(sfBalance, balance - potentialTotalSpend);
+            // The transaction's sequence/ticket will be valid when the other
+            // transactions in the queue have been processed. If the tx has a
+            // sequence, set the account to match it. If it has a ticket, use
+            // the next queueable sequence, which is the closest approximation
+            // to the most successful case.
+            sleBump->at(sfSequence) = txSeqProx.isSeq()
+                ? txSeqProx.value()
+                : nextQueuableSeqImpl(sleAccount, lock).value();
         }
     }
 
@@ -1134,8 +1142,8 @@ TxQ::apply(
     // Note that earlier code has already verified that the sequence/ticket
     // is valid.  So we use a special entry point that runs all of the
     // preclaim checks with the exception of the sequence check.
-    auto const pcresult = ForTxQ::preclaimWithoutSeqCheck(
-        pfresult, app, multiTxn ? multiTxn->openView : view);
+    auto const pcresult =
+        preclaim(pfresult, app, multiTxn ? multiTxn->openView : view);
     if (!pcresult.likelyToClaimFee)
         return {pcresult.ter, false};
 

src/ripple/app/tx/applySteps.h

@@ -284,29 +284,6 @@ preclaim(
     Application& app,
     OpenView const& view);
 
-// There are two special entry points that are only intended for use by the
-// TxQ.  To help enforce that this class contains the two functions as
-// static members.
-class ForTxQ
-{
-private:
-    friend TxQ;
-
-    // This entry point runs all of the preclaim checks with the lone exception
-    // of verifying Sequence or Ticket validity.  This allows the TxQ to
-    // perform its own similar checks without needing to construct a bogus view.
-    static PreclaimResult
-    preclaimWithoutSeqCheck(
-        PreflightResult const& preflightResult,
-        Application& app,
-        OpenView const& view);
-
-    // Checks the sequence number explicitly.  Used by the TxQ in the case
-    // where the preclaim sequence number check was skipped earlier.
-    static TER
-    seqCheck(OpenView& view, STTx const& tx, beast::Journal j);
-};
-
 /** Compute only the expected base fee for a transaction.
 
     Base fees are transaction specific, so any calculation

src/ripple/app/tx/impl/CancelOffer.cpp

@@ -40,8 +40,7 @@ CancelOffer::preflight(PreflightContext const& ctx)
         return temINVALID_FLAG;
     }
 
-    auto const seq = ctx.tx.getFieldU32(sfOfferSequence);
-    if (!seq)
+    if (!ctx.tx[sfOfferSequence])
     {
         JLOG(ctx.j.trace()) << "CancelOffer::preflight: missing sequence";
         return temBAD_SEQUENCE;

src/ripple/app/tx/impl/CreateOffer.cpp

@@ -75,9 +75,8 @@ CreateOffer::preflight(PreflightContext const& ctx)
         return temBAD_EXPIRATION;
     }
 
-    bool const bHaveCancel(tx.isFieldPresent(sfOfferSequence));
-
-    if (bHaveCancel && (tx.getFieldU32(sfOfferSequence) == 0))
+    if (auto const cancelSequence = tx[~sfOfferSequence];
+        cancelSequence && *cancelSequence == 0)
     {
         JLOG(j.debug()) << "Malformed offer: bad cancel sequence";
         return temBAD_SEQUENCE;

src/ripple/app/tx/impl/Transactor.cpp

@@ -247,7 +247,7 @@ Transactor::checkSeqProxy(
     {
         JLOG(j.trace())
             << "applyTransaction: delay: source account does not exist "
-            << toBase58(tx.getAccountID(sfAccount));
+            << toBase58(id);
         return terNO_ACCOUNT;
     }
 
@@ -305,7 +305,7 @@ Transactor::checkPriorTxAndLastLedger(PreclaimContext const& ctx)
     {
         JLOG(ctx.j.trace())
             << "applyTransaction: delay: source account does not exist "
-            << toBase58(ctx.tx.getAccountID(sfAccount));
+            << toBase58(id);
         return terNO_ACCOUNT;
     }
 

src/ripple/app/tx/impl/applySteps.cpp

@@ -138,20 +138,13 @@ invoke_preflight(PreflightContext const& ctx)
     }
 }
 
-// The TxQ needs to be able to bypass checking the Sequence or Ticket
-// The enum provides a self-documenting way to do that
-enum class SeqCheck : bool {
-    no = false,
-    yes = true,
-};
-
 /* invoke_preclaim<T> uses name hiding to accomplish
     compile-time polymorphism of (presumably) static
     class functions for Transactor and derived classes.
 */
 template <class T>
 static TER
-invoke_preclaim(PreclaimContext const& ctx, SeqCheck seqChk)
+invoke_preclaim(PreclaimContext const& ctx)
 {
     // If the transactor requires a valid account and the transaction doesn't
     // list one, preflight will have already a flagged a failure.
@@ -159,14 +152,10 @@ invoke_preclaim(PreclaimContext const& ctx, SeqCheck seqChk)
 
     if (id != beast::zero)
     {
-        TER result{tesSUCCESS};
-        if (seqChk == SeqCheck::yes)
-        {
-            result = T::checkSeqProxy(ctx.view, ctx.tx, ctx.j);
+        TER result = T::checkSeqProxy(ctx.view, ctx.tx, ctx.j);
 
-            if (result != tesSUCCESS)
-                return result;
-        }
+        if (result != tesSUCCESS)
+            return result;
 
         result = T::checkPriorTxAndLastLedger(ctx);
 
@@ -188,112 +177,52 @@ invoke_preclaim(PreclaimContext const& ctx, SeqCheck seqChk)
 }
 
 static TER
-invoke_preclaim(PreclaimContext const& ctx, SeqCheck seqChk)
+invoke_preclaim(PreclaimContext const& ctx)
 {
     switch (ctx.tx.getTxnType())
     {
         case ttACCOUNT_DELETE:
-            return invoke_preclaim<DeleteAccount>(ctx, seqChk);
+            return invoke_preclaim<DeleteAccount>(ctx);
         case ttACCOUNT_SET:
-            return invoke_preclaim<SetAccount>(ctx, seqChk);
+            return invoke_preclaim<SetAccount>(ctx);
         case ttCHECK_CANCEL:
-            return invoke_preclaim<CancelCheck>(ctx, seqChk);
+            return invoke_preclaim<CancelCheck>(ctx);
         case ttCHECK_CASH:
-            return invoke_preclaim<CashCheck>(ctx, seqChk);
+            return invoke_preclaim<CashCheck>(ctx);
         case ttCHECK_CREATE:
-            return invoke_preclaim<CreateCheck>(ctx, seqChk);
+            return invoke_preclaim<CreateCheck>(ctx);
         case ttDEPOSIT_PREAUTH:
-            return invoke_preclaim<DepositPreauth>(ctx, seqChk);
+            return invoke_preclaim<DepositPreauth>(ctx);
         case ttOFFER_CANCEL:
-            return invoke_preclaim<CancelOffer>(ctx, seqChk);
+            return invoke_preclaim<CancelOffer>(ctx);
         case ttOFFER_CREATE:
-            return invoke_preclaim<CreateOffer>(ctx, seqChk);
+            return invoke_preclaim<CreateOffer>(ctx);
         case ttESCROW_CREATE:
-            return invoke_preclaim<EscrowCreate>(ctx, seqChk);
+            return invoke_preclaim<EscrowCreate>(ctx);
         case ttESCROW_FINISH:
-            return invoke_preclaim<EscrowFinish>(ctx, seqChk);
+            return invoke_preclaim<EscrowFinish>(ctx);
         case ttESCROW_CANCEL:
-            return invoke_preclaim<EscrowCancel>(ctx, seqChk);
+            return invoke_preclaim<EscrowCancel>(ctx);
         case ttPAYCHAN_CLAIM:
-            return invoke_preclaim<PayChanClaim>(ctx, seqChk);
+            return invoke_preclaim<PayChanClaim>(ctx);
         case ttPAYCHAN_CREATE:
-            return invoke_preclaim<PayChanCreate>(ctx, seqChk);
+            return invoke_preclaim<PayChanCreate>(ctx);
         case ttPAYCHAN_FUND:
-            return invoke_preclaim<PayChanFund>(ctx, seqChk);
+            return invoke_preclaim<PayChanFund>(ctx);
         case ttPAYMENT:
-            return invoke_preclaim<Payment>(ctx, seqChk);
+            return invoke_preclaim<Payment>(ctx);
         case ttREGULAR_KEY_SET:
-            return invoke_preclaim<SetRegularKey>(ctx, seqChk);
+            return invoke_preclaim<SetRegularKey>(ctx);
         case ttSIGNER_LIST_SET:
-            return invoke_preclaim<SetSignerList>(ctx, seqChk);
+            return invoke_preclaim<SetSignerList>(ctx);
         case ttTICKET_CREATE:
-            return invoke_preclaim<CreateTicket>(ctx, seqChk);
+            return invoke_preclaim<CreateTicket>(ctx);
         case ttTRUST_SET:
-            return invoke_preclaim<SetTrust>(ctx, seqChk);
+            return invoke_preclaim<SetTrust>(ctx);
         case ttAMENDMENT:
         case ttFEE:
         case ttUNL_MODIFY:
-            return invoke_preclaim<Change>(ctx, seqChk);
-        default:
-            assert(false);
-            return temUNKNOWN;
-    }
-}
-
-template <class T>
-static TER
-invoke_seqCheck(ReadView const& view, STTx const& tx, beast::Journal j)
-{
-    return T::checkSeqProxy(view, tx, j);
-}
-
-TER
-ForTxQ::seqCheck(OpenView& view, STTx const& tx, beast::Journal j)
-{
-    switch (tx.getTxnType())
-    {
-        case ttACCOUNT_DELETE:
-            return invoke_seqCheck<DeleteAccount>(view, tx, j);
-        case ttACCOUNT_SET:
-            return invoke_seqCheck<SetAccount>(view, tx, j);
-        case ttCHECK_CANCEL:
-            return invoke_seqCheck<CancelCheck>(view, tx, j);
-        case ttCHECK_CASH:
-            return invoke_seqCheck<CashCheck>(view, tx, j);
-        case ttCHECK_CREATE:
-            return invoke_seqCheck<CreateCheck>(view, tx, j);
-        case ttDEPOSIT_PREAUTH:
-            return invoke_seqCheck<DepositPreauth>(view, tx, j);
-        case ttOFFER_CANCEL:
-            return invoke_seqCheck<CancelOffer>(view, tx, j);
-        case ttOFFER_CREATE:
-            return invoke_seqCheck<CreateOffer>(view, tx, j);
-        case ttESCROW_CREATE:
-            return invoke_seqCheck<EscrowCreate>(view, tx, j);
-        case ttESCROW_FINISH:
-            return invoke_seqCheck<EscrowFinish>(view, tx, j);
-        case ttESCROW_CANCEL:
-            return invoke_seqCheck<EscrowCancel>(view, tx, j);
-        case ttPAYCHAN_CLAIM:
-            return invoke_seqCheck<PayChanClaim>(view, tx, j);
-        case ttPAYCHAN_CREATE:
-            return invoke_seqCheck<PayChanCreate>(view, tx, j);
-        case ttPAYCHAN_FUND:
-            return invoke_seqCheck<PayChanFund>(view, tx, j);
-        case ttPAYMENT:
-            return invoke_seqCheck<Payment>(view, tx, j);
-        case ttREGULAR_KEY_SET:
-            return invoke_seqCheck<SetRegularKey>(view, tx, j);
-        case ttSIGNER_LIST_SET:
-            return invoke_seqCheck<SetSignerList>(view, tx, j);
-        case ttTICKET_CREATE:
-            return invoke_seqCheck<CreateTicket>(view, tx, j);
-        case ttTRUST_SET:
-            return invoke_seqCheck<SetTrust>(view, tx, j);
-        case ttAMENDMENT:
-        case ttFEE:
-        case ttUNL_MODIFY:
-            return invoke_seqCheck<Change>(view, tx, j);
+            return invoke_preclaim<Change>(ctx);
         default:
             assert(false);
             return temUNKNOWN;
@@ -505,11 +434,11 @@ preflight(
     }
 }
 
-PreclaimResult static preclaim(
+PreclaimResult
+preclaim(
     PreflightResult const& preflightResult,
     Application& app,
-    OpenView const& view,
-    SeqCheck seqChk)
+    OpenView const& view)
 {
     boost::optional<PreclaimContext const> ctx;
     if (preflightResult.rules != view.rules())
@@ -542,7 +471,7 @@ PreclaimResult static preclaim(
     {
         if (ctx->preflightResult != tesSUCCESS)
             return {*ctx, ctx->preflightResult};
-        return {*ctx, invoke_preclaim(*ctx, seqChk)};
+        return {*ctx, invoke_preclaim(*ctx)};
     }
     catch (std::exception const& e)
     {
@@ -551,24 +480,6 @@ PreclaimResult static preclaim(
     }
 }
 
-PreclaimResult
-preclaim(
-    PreflightResult const& preflightResult,
-    Application& app,
-    OpenView const& view)
-{
-    return preclaim(preflightResult, app, view, SeqCheck::yes);
-}
-
-PreclaimResult
-ForTxQ::preclaimWithoutSeqCheck(
-    PreflightResult const& preflightResult,
-    Application& app,
-    OpenView const& view)
-{
-    return preclaim(preflightResult, app, view, SeqCheck::no);
-}
-
 FeeUnit64
 calculateBaseFee(ReadView const& view, STTx const& tx)
 {

src/ripple/protocol/STObject.h

@@ -477,31 +477,35 @@ public:
     const STArray&
     getFieldArray(SField const& field) const;
 
-    /** Return the value of a field.
-
-        Throws:
-
-            STObject::FieldErr if the field is
-            not present.
+    /** Get the value of a field.
+        @param A TypedField built from an SField value representing the desired
+            object field. In typical use, the TypedField will be implicitly
+            constructed.
+        @return The value of the specified field.
+        @throws STObject::FieldErr if the field is not present.
     */
     template <class T>
     typename T::value_type
     operator[](TypedField<T> const& f) const;
 
-    /** Return the value of a field as boost::optional
+    /** Get the value of a field as boost::optional
 
-        @return boost::none if the field is not present.
+        @param An OptionaledField built from an SField value representing the
+           desired object field. In typical use, the OptionaledField will be
+           constructed by using the ~ operator on an SField.
+        @return boost::none if the field is not present, else the value of the
+           specified field.
     */
     template <class T>
     boost::optional<std::decay_t<typename T::value_type>>
     operator[](OptionaledField<T> const& of) const;
 
-    /** Return a modifiable field value.
-
-        Throws:
-
-            STObject::FieldErr if the field is
-            not present.
+    /** Get a modifiable field value.
+        @param A TypedField built from an SField value representing the desired
+            object field. In typical use, the TypedField will be implicitly
+            constructed.
+        @return A modifiable reference to the value of the specified field.
+        @throws STObject::FieldErr if the field is not present.
     */
     template <class T>
     ValueProxy<T>
@@ -509,13 +513,64 @@ public:
 
     /** Return a modifiable field value as boost::optional
 
-        The return value equals boost::none if the
-        field is not present.
+        @param An OptionaledField built from an SField value representing the
+            desired object field. In typical use, the OptionaledField will be
+            constructed by using the ~ operator on an SField.
+        @return Transparent proxy object to an `optional` holding a modifiable
+            reference to the value of the specified field. Returns boost::none
+            if the field is not present.
     */
     template <class T>
     OptionalProxy<T>
     operator[](OptionaledField<T> const& of);
 
+    /** Get the value of a field.
+        @param A TypedField built from an SField value representing the desired
+            object field. In typical use, the TypedField will be implicitly
+            constructed.
+        @return The value of the specified field.
+        @throws STObject::FieldErr if the field is not present.
+    */
+    template <class T>
+    typename T::value_type
+    at(TypedField<T> const& f) const;
+
+    /** Get the value of a field as boost::optional
+
+        @param An OptionaledField built from an SField value representing the
+           desired object field. In typical use, the OptionaledField will be
+           constructed by using the ~ operator on an SField.
+        @return boost::none if the field is not present, else the value of the
+           specified field.
+    */
+    template <class T>
+    boost::optional<std::decay_t<typename T::value_type>>
+    at(OptionaledField<T> const& of) const;
+
+    /** Get a modifiable field value.
+        @param A TypedField built from an SField value representing the desired
+            object field. In typical use, the TypedField will be implicitly
+            constructed.
+        @return A modifiable reference to the value of the specified field.
+        @throws STObject::FieldErr if the field is not present.
+    */
+    template <class T>
+    ValueProxy<T>
+    at(TypedField<T> const& f);
+
+    /** Return a modifiable field value as boost::optional
+
+        @param An OptionaledField built from an SField value representing the
+            desired object field. In typical use, the OptionaledField will be
+            constructed by using the ~ operator on an SField.
+        @return Transparent proxy object to an `optional` holding a modifiable
+            reference to the value of the specified field. Returns boost::none
+            if the field is not present.
+    */
+    template <class T>
+    OptionalProxy<T>
+    at(OptionaledField<T> const& of);
+
     /** Set a field.
         if the field already exists, it is replaced.
     */
@@ -926,6 +981,34 @@ STObject::OptionalProxy<T>::optional_value() const -> optional_type
 template <class T>
 typename T::value_type
 STObject::operator[](TypedField<T> const& f) const
+{
+    return at(f);
+}
+
+template <class T>
+boost::optional<std::decay_t<typename T::value_type>>
+STObject::operator[](OptionaledField<T> const& of) const
+{
+    return at(of);
+}
+
+template <class T>
+inline auto
+STObject::operator[](TypedField<T> const& f) -> ValueProxy<T>
+{
+    return at(f);
+}
+
+template <class T>
+inline auto
+STObject::operator[](OptionaledField<T> const& of) -> OptionalProxy<T>
+{
+    return at(of);
+}
+
+template <class T>
+typename T::value_type
+STObject::at(TypedField<T> const& f) const
 {
     auto const b = peekAtPField(f);
     if (!b)
@@ -951,7 +1034,7 @@ STObject::operator[](TypedField<T> const& f) const
 
 template <class T>
 boost::optional<std::decay_t<typename T::value_type>>
-STObject::operator[](OptionaledField<T> const& of) const
+STObject::at(OptionaledField<T> const& of) const
 {
     auto const b = peekAtPField(*of.f);
     if (!b)
@@ -971,14 +1054,14 @@ STObject::operator[](OptionaledField<T> const& of) const
 
 template <class T>
 inline auto
-STObject::operator[](TypedField<T> const& f) -> ValueProxy<T>
+STObject::at(TypedField<T> const& f) -> ValueProxy<T>
 {
     return ValueProxy<T>(this, &f);
 }
 
 template <class T>
 inline auto
-STObject::operator[](OptionaledField<T> const& of) -> OptionalProxy<T>
+STObject::at(OptionaledField<T> const& of) -> OptionalProxy<T>
 {
     return OptionalProxy<T>(this, of.f);
 }