From 77ec62e9c8d7252279cf0edd7b6e2faf80ff5b4e Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Thu, 8 Oct 2020 19:15:16 -0400 Subject: [PATCH] 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 | 12 +- src/ripple/app/tx/applySteps.h | 23 ---- src/ripple/app/tx/impl/CancelOffer.cpp | 3 +- src/ripple/app/tx/impl/CreateOffer.cpp | 5 +- src/ripple/app/tx/impl/Transactor.cpp | 4 +- src/ripple/app/tx/impl/applySteps.cpp | 147 +++++-------------------- src/ripple/protocol/STObject.h | 121 ++++++++++++++++---- 7 files changed, 146 insertions(+), 169 deletions(-) diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index d3f12f39d..07668c1a6 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/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}; diff --git a/src/ripple/app/tx/applySteps.h b/src/ripple/app/tx/applySteps.h index b9e731697..9993ba0c5 100644 --- a/src/ripple/app/tx/applySteps.h +++ b/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 diff --git a/src/ripple/app/tx/impl/CancelOffer.cpp b/src/ripple/app/tx/impl/CancelOffer.cpp index a7a766537..89d6b62c7 100644 --- a/src/ripple/app/tx/impl/CancelOffer.cpp +++ b/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; diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index e7006c4cc..e952ac963 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/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; diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 4810e4a78..5c777afd1 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/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; } diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index 1e6e35d9b..c058b2967 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/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 uses name hiding to accomplish compile-time polymorphism of (presumably) static class functions for Transactor and derived classes. */ template 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(ctx, seqChk); + return invoke_preclaim(ctx); case ttACCOUNT_SET: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttCHECK_CANCEL: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttCHECK_CASH: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttCHECK_CREATE: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttDEPOSIT_PREAUTH: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttOFFER_CANCEL: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttOFFER_CREATE: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttESCROW_CREATE: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttESCROW_FINISH: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttESCROW_CANCEL: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttPAYCHAN_CLAIM: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttPAYCHAN_CREATE: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttPAYCHAN_FUND: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttPAYMENT: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttREGULAR_KEY_SET: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttSIGNER_LIST_SET: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttTICKET_CREATE: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttTRUST_SET: - return invoke_preclaim(ctx, seqChk); + return invoke_preclaim(ctx); case ttAMENDMENT: case ttFEE: case ttUNL_MODIFY: - return invoke_preclaim(ctx, seqChk); - default: - assert(false); - return temUNKNOWN; - } -} - -template -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(view, tx, j); - case ttACCOUNT_SET: - return invoke_seqCheck(view, tx, j); - case ttCHECK_CANCEL: - return invoke_seqCheck(view, tx, j); - case ttCHECK_CASH: - return invoke_seqCheck(view, tx, j); - case ttCHECK_CREATE: - return invoke_seqCheck(view, tx, j); - case ttDEPOSIT_PREAUTH: - return invoke_seqCheck(view, tx, j); - case ttOFFER_CANCEL: - return invoke_seqCheck(view, tx, j); - case ttOFFER_CREATE: - return invoke_seqCheck(view, tx, j); - case ttESCROW_CREATE: - return invoke_seqCheck(view, tx, j); - case ttESCROW_FINISH: - return invoke_seqCheck(view, tx, j); - case ttESCROW_CANCEL: - return invoke_seqCheck(view, tx, j); - case ttPAYCHAN_CLAIM: - return invoke_seqCheck(view, tx, j); - case ttPAYCHAN_CREATE: - return invoke_seqCheck(view, tx, j); - case ttPAYCHAN_FUND: - return invoke_seqCheck(view, tx, j); - case ttPAYMENT: - return invoke_seqCheck(view, tx, j); - case ttREGULAR_KEY_SET: - return invoke_seqCheck(view, tx, j); - case ttSIGNER_LIST_SET: - return invoke_seqCheck(view, tx, j); - case ttTICKET_CREATE: - return invoke_seqCheck(view, tx, j); - case ttTRUST_SET: - return invoke_seqCheck(view, tx, j); - case ttAMENDMENT: - case ttFEE: - case ttUNL_MODIFY: - return invoke_seqCheck(view, tx, j); + return invoke_preclaim(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 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) { diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 9cd407efc..4ac67ded5 100644 --- a/src/ripple/protocol/STObject.h +++ b/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 typename T::value_type operator[](TypedField 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 boost::optional> operator[](OptionaledField 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 ValueProxy @@ -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 OptionalProxy operator[](OptionaledField 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 + typename T::value_type + at(TypedField 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 + boost::optional> + at(OptionaledField 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 + ValueProxy + at(TypedField 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 + OptionalProxy + at(OptionaledField const& of); + /** Set a field. if the field already exists, it is replaced. */ @@ -926,6 +981,34 @@ STObject::OptionalProxy::optional_value() const -> optional_type template typename T::value_type STObject::operator[](TypedField const& f) const +{ + return at(f); +} + +template +boost::optional> +STObject::operator[](OptionaledField const& of) const +{ + return at(of); +} + +template +inline auto +STObject::operator[](TypedField const& f) -> ValueProxy +{ + return at(f); +} + +template +inline auto +STObject::operator[](OptionaledField const& of) -> OptionalProxy +{ + return at(of); +} + +template +typename T::value_type +STObject::at(TypedField const& f) const { auto const b = peekAtPField(f); if (!b) @@ -951,7 +1034,7 @@ STObject::operator[](TypedField const& f) const template boost::optional> -STObject::operator[](OptionaledField const& of) const +STObject::at(OptionaledField const& of) const { auto const b = peekAtPField(*of.f); if (!b) @@ -971,14 +1054,14 @@ STObject::operator[](OptionaledField const& of) const template inline auto -STObject::operator[](TypedField const& f) -> ValueProxy +STObject::at(TypedField const& f) -> ValueProxy { return ValueProxy(this, &f); } template inline auto -STObject::operator[](OptionaledField const& of) -> OptionalProxy +STObject::at(OptionaledField const& of) -> OptionalProxy { return OptionalProxy(this, of.f); }