From 182f570f2474df29964d6541ae6cfb31401d1dcf Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 31 Jul 2015 14:33:33 -0700 Subject: [PATCH] Reduce Transactor virtual interface --- src/ripple/app/tx/impl/Change.cpp | 52 +++++++----------------- src/ripple/app/tx/impl/Change.h | 8 +--- src/ripple/app/tx/impl/Transactor.cpp | 58 ++++++++++----------------- src/ripple/app/tx/impl/Transactor.h | 19 ++------- 4 files changed, 42 insertions(+), 95 deletions(-) diff --git a/src/ripple/app/tx/impl/Change.cpp b/src/ripple/app/tx/impl/Change.cpp index 737cfeeb7..af14074fe 100644 --- a/src/ripple/app/tx/impl/Change.cpp +++ b/src/ripple/app/tx/impl/Change.cpp @@ -34,17 +34,29 @@ Change::preflight (PreflightContext const& ctx) auto account = ctx.tx.getAccountID(sfAccount); if (account != zero) { - JLOG(ctx.j.warning) << "Bad source id"; + JLOG(ctx.j.warning) << "Change: Bad source id"; return temBAD_SRC_ACCOUNT; } auto const fee = ctx.tx.getTransactionFee (); if (!fee.native () || fee != beast::zero) { - JLOG(ctx.j.warning) << "Non-zero fee"; + JLOG(ctx.j.warning) << "Change: Non-zero fee"; return temBAD_FEE; } + if (!ctx.tx.getSigningPubKey ().empty () || !ctx.tx.getSignature ().empty ()) + { + JLOG(ctx.j.warning) << "Change: Bad signature"; + return temBAD_SIGNATURE; + } + + if (ctx.tx.getSequence () != 0 || ctx.tx.isFieldPresent (sfPreviousTxnID)) + { + JLOG(ctx.j.warning) << "Change: Bad sequence"; + return temBAD_SEQUENCE; + } + return tesSUCCESS; } @@ -68,42 +80,6 @@ Change::doApply() return temUNKNOWN; } -TER -Change::checkSign() -{ - if (!tx().getSigningPubKey ().empty () || !tx().getSignature ().empty ()) - { - j_.warning << "Bad signature"; - return temBAD_SIGNATURE; - } - - return tesSUCCESS; -} - -TER -Change::checkSeq() -{ - if ((tx().getSequence () != 0) || tx().isFieldPresent (sfPreviousTxnID)) - { - j_.warning << "Bad sequence"; - return temBAD_SEQUENCE; - } - - return tesSUCCESS; -} - -TER -Change::payFee() -{ - if (tx().getTransactionFee () != beast::zero) - { - j_.warning << "Non-zero fee"; - return temBAD_FEE; - } - - return tesSUCCESS; -} - void Change::preCompute() { diff --git a/src/ripple/app/tx/impl/Change.h b/src/ripple/app/tx/impl/Change.h index 6f4de284b..14d4c2533 100644 --- a/src/ripple/app/tx/impl/Change.h +++ b/src/ripple/app/tx/impl/Change.h @@ -43,9 +43,6 @@ public: preflight (PreflightContext const& ctx); TER doApply () override; - TER checkSign () override; - TER checkSeq () override; - TER payFee () override; void preCompute() override; private: @@ -53,10 +50,9 @@ private: TER applyFee (); - // VFALCO TODO Can this be removed? - bool mustHaveValidAccount () override + std::uint64_t calculateBaseFee () override { - return false; + return 0; } }; diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index a3cd26d44..3589ea056 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -96,13 +96,6 @@ Transactor::Transactor( { } -void Transactor::calculateFee () -{ - mFeeDue = STAmount (getApp().getFeeTrack().scaleFeeLoad( - calculateBaseFee(), view().fees().base, - view().fees().units, view().flags() & tapADMIN)); -} - std::uint64_t Transactor::calculateBaseFee () { // Returns the fee in fee units @@ -113,22 +106,18 @@ TER Transactor::payFee () { STAmount saPaid = tx().getTransactionFee (); - if (!isLegalNet (saPaid)) - return temBAD_AMOUNT; + if (!isLegalNet (saPaid) || saPaid < zero) + return temBAD_FEE; // Only check fee is sufficient when the ledger is open. if (view().open() && saPaid < mFeeDue) { JLOG(j_.trace) << "Insufficient fee paid: " << saPaid.getText () << "/" << mFeeDue.getText (); - return telINSUF_FEE_P; } - if (saPaid < zero || !saPaid.native ()) - return temBAD_FEE; - - if (!saPaid) + if (saPaid == zero) return tesSUCCESS; auto const sle = view().peek( @@ -216,44 +205,41 @@ TER Transactor::apply () { preCompute(); - // Find source account + // If the transactor requires a valid account and the transaction doesn't + // list one, preflight will have already a flagged a failure. auto const sle = view().peek (keylet::account(account_)); - calculateFee (); - - // If are only forwarding, due to resource limitations, we might verifying - // only some transactions, this would be probabilistic. - if (!sle) + if (sle == nullptr && account_ != zero) { - if (mustHaveValidAccount ()) - { - JLOG(j_.trace) << - "applyTransaction: delay: source account does not exist " << - toBase58(tx().getAccountID(sfAccount)); - return terNO_ACCOUNT; - } + JLOG (j_.trace) << + "apply: source account " << toBase58(account_) << " not found."; + return terNO_ACCOUNT; } - else + + mFeeDue = STAmount (getApp().getFeeTrack().scaleFeeLoad( + calculateBaseFee(), view().fees().base, + view().fees().units, view().flags() & tapADMIN)); + + if (sle) { mPriorBalance = sle->getFieldAmount (sfBalance); mSourceBalance = mPriorBalance; mHasAuthKey = sle->isFieldPresent (sfRegularKey); - } - auto terResult = checkSeq (); + auto terResult = checkSeq (); - if (terResult != tesSUCCESS) return terResult; + if (terResult != tesSUCCESS) return terResult; - terResult = payFee (); + terResult = payFee (); - if (terResult != tesSUCCESS) return terResult; + if (terResult != tesSUCCESS) return terResult; - terResult = checkSign (); + terResult = checkSign (); - if (terResult != tesSUCCESS) return terResult; + if (terResult != tesSUCCESS) return terResult; - if (sle) view().update (sle); + } return doApply (); } diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index 8f11e4f1c..43854d4bd 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/src/ripple/app/tx/impl/Transactor.h @@ -94,28 +94,17 @@ protected: explicit Transactor (ApplyContext& ctx); - void calculateFee (); - - // VFALCO This is the equivalent of dynamic_cast - // to discover the type of the derived class, - // and therefore bad. - virtual - bool - mustHaveValidAccount() - { - return true; - } - // Returns the fee, not scaled for load (Should be in fee units. FIXME) virtual std::uint64_t calculateBaseFee (); virtual void preCompute(); - virtual TER checkSeq (); - virtual TER payFee (); - virtual TER checkSign (); + virtual TER doApply () = 0; private: + TER checkSeq (); + TER checkSign (); + TER payFee (); TER checkSingleSign (); TER checkMultiSign (); };