diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index e59277e09..b0f9203be 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -654,7 +654,7 @@ RCLConsensus::Adaptor::buildLCL( if (replayData) { assert(replayData->parent()->info().hash == previousLedger.id()); - return buildLedger(*replayData, tapNO_CHECK_SIGN, app_, j_); + return buildLedger(*replayData, tapNONE, app_, j_); } return buildLedger( previousLedger.ledger_, diff --git a/src/ripple/app/ledger/impl/BuildLedger.cpp b/src/ripple/app/ledger/impl/BuildLedger.cpp index 55754483f..e430818b4 100644 --- a/src/ripple/app/ledger/impl/BuildLedger.cpp +++ b/src/ripple/app/ledger/impl/BuildLedger.cpp @@ -139,7 +139,7 @@ applyTransactions( try { switch (applyTransaction( - app, view, *it->second, certainRetry, tapNO_CHECK_SIGN, j)) + app, view, *it->second, certainRetry, tapNONE, j)) { case ApplyResult::Success: it = retriableTxs.erase(it); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 8d0abb0b0..d2f3107b7 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -1028,8 +1028,8 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) { for (TransactionStatus& e : transactions) { - // we check before addingto the batch - ApplyFlags flags = tapNO_CHECK_SIGN; + // we check before adding to the batch + ApplyFlags flags = tapNONE; if (e.admin) flags = flags | tapUNLIMITED; diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 95e3b6a9b..1657e5deb 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -335,7 +335,7 @@ private: PreflightResult const& pfresult); std::pair - apply(Application& app, OpenView& view); + apply(Application& app, OpenView& view, beast::Journal j); }; class GreaterFee diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index a88b681dd..b3ed33a50 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -262,7 +262,7 @@ TxQ::MaybeTx::MaybeTx( } std::pair -TxQ::MaybeTx::apply(Application& app, OpenView& view) +TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j) { boost::optional saved; if (view.rules().enabled(fix1513)) @@ -272,6 +272,10 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view) if (pfresult->rules != view.rules() || pfresult->flags != flags) { + JLOG(j.debug()) << "Queued transaction " << + txID << " rules or flags have changed. Flags from " << + pfresult->flags << " to " << flags ; + pfresult.emplace( preflight(app, view.rules(), pfresult->tx, @@ -504,7 +508,7 @@ TxQ::tryClearAccountQueue(Application& app, OpenView& view, // Attempt to apply the queued transactions. for (auto it = beginTxIter; it != endTxIter; ++it) { - auto txResult = it->second.apply(app, view); + auto txResult = it->second.apply(app, view, j); // Succeed or fail, use up a retry, because if the overall // process fails, we want the attempt to count. If it all // succeeds, the MaybeTx will be destructed, so it'll be @@ -1009,7 +1013,7 @@ TxQ::apply(Application& app, OpenView& view, std::tie(txnResult, didApply) = doApply(pcresult, app, view); - JLOG(j_.trace()) << "Transaction " << + JLOG(j_.trace()) << "New transaction " << transactionID << (didApply ? " applied successfully with " : " failed with ") << @@ -1111,6 +1115,17 @@ TxQ::apply(Application& app, OpenView& view, (void)created; assert(created); } + // Modify the flags for use when coming out of the queue. + // These changes _may_ cause an extra `preflight`, but as long as + // the `HashRouter` still knows about the transaction, the signature + // will not be checked again, so the cost should be minimal. + + // Don't allow soft failures, which can lead to retries + flags &= ~tapRETRY; + + // Don't queue because we're already in the queue + flags &= ~tapPREFER_QUEUE; + auto& candidate = accountIter->second.add( { tx, transactionID, feeLevelPaid, flags, pfresult }); /* Normally we defer figuring out the consequences until @@ -1122,8 +1137,10 @@ TxQ::apply(Application& app, OpenView& view, // Then index it into the byFee lookup. byFee_.insert(candidate); JLOG(j_.debug()) << "Added transaction " << candidate.txID << + " with result " << transToken(pfresult.ter) << " from " << (accountExists ? "existing" : "new") << - " account " << candidate.account << " to queue."; + " account " << candidate.account << " to queue." << + " Flags: " << flags; return { terQUEUED, false }; } @@ -1278,14 +1295,15 @@ TxQ::accept(Application& app, TER txnResult; bool didApply; - std::tie(txnResult, didApply) = candidateIter->apply(app, view); + std::tie(txnResult, didApply) = candidateIter->apply(app, view, j_); if (didApply) { // Remove the candidate from the queue JLOG(j_.debug()) << "Queued transaction " << candidateIter->txID << - " applied successfully. Remove from queue."; + " applied successfully with " << + transToken(txnResult) << ". Remove from queue."; candidateIter = eraseAndAdvance(candidateIter); ledgerChanged = true; @@ -1304,9 +1322,12 @@ TxQ::accept(Application& app, } else { - JLOG(j_.debug()) << "Transaction " << + JLOG(j_.debug()) << "Queued transaction " << candidateIter->txID << " failed with " << - transToken(txnResult) << ". Leave in queue."; + transToken(txnResult) << ". Leave in queue." << + " Applied: " << didApply << + ". Flags: " << + candidateIter->flags; if (account.retryPenalty && candidateIter->retriesRemaining > 2) candidateIter->retriesRemaining = 1; diff --git a/src/ripple/app/tx/applySteps.h b/src/ripple/app/tx/applySteps.h index 1cba92d8d..0f4768fe7 100644 --- a/src/ripple/app/tx/applySteps.h +++ b/src/ripple/app/tx/applySteps.h @@ -28,6 +28,16 @@ namespace ripple { class Application; class STTx; +/** Return true if the transaction can claim a fee (tec), + and the `ApplyFlags` do not allow soft failures. + */ +inline +bool +isTecClaimHardFail(TER ter, ApplyFlags flags) +{ + return isTecClaim(ter) && !(flags & tapRETRY); +} + /** Describes the results of the `preflight` check @note All members are const to make it more difficult @@ -99,7 +109,7 @@ public: , j(ctx_.j) , ter(ter_) , likelyToClaimFee(ter == tesSUCCESS - || isTecClaim(ter)) + || isTecClaimHardFail(ter, flags)) { } @@ -268,4 +278,4 @@ doApply(PreclaimResult const& preclaimResult, } -#endif \ No newline at end of file +#endif diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 632d62880..708fe3648 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -88,16 +88,13 @@ preflight1 (PreflightContext const& ctx) NotTEC preflight2 (PreflightContext const& ctx) { - if(!( ctx.flags & tapNO_CHECK_SIGN)) + auto const sigValid = checkValidity(ctx.app.getHashRouter(), + ctx.tx, ctx.rules, ctx.app.config()); + if (sigValid.first == Validity::SigBad) { - auto const sigValid = checkValidity(ctx.app.getHashRouter(), - ctx.tx, ctx.rules, ctx.app.config()); - if (sigValid.first == Validity::SigBad) - { - JLOG(ctx.j.debug()) << - "preflight2: bad signature. " << sigValid.second; - return temINVALID; - } + JLOG(ctx.j.debug()) << + "preflight2: bad signature. " << sigValid.second; + return temINVALID; } return tesSUCCESS; } @@ -638,7 +635,7 @@ Transactor::operator()() result = tecOVERSIZE; if ((result == tecOVERSIZE) || - (isTecClaim (result) && !(view().flags() & tapRETRY))) + (isTecClaimHardFail (result, view().flags()))) { JLOG(j_.trace()) << "reapplying because of " << transToken(result); diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index 7ec85f48a..9328b15ee 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -30,9 +30,6 @@ enum ApplyFlags { tapNONE = 0x00, - // Signature already checked - tapNO_CHECK_SIGN = 0x01, - // This is not the transaction's last pass // Transaction can be retried, soft failures allowed tapRETRY = 0x20, @@ -46,24 +43,63 @@ enum ApplyFlags tapUNLIMITED = 0x400, }; -inline +constexpr ApplyFlags operator|(ApplyFlags const& lhs, ApplyFlags const& rhs) { return static_cast( - static_cast(lhs) | - static_cast(rhs)); + static_cast>(lhs) | + static_cast>(rhs)); } -inline +static_assert((tapPREFER_QUEUE | tapRETRY) == static_cast(0x60), + "ApplyFlags operator |"); +static_assert((tapRETRY | tapPREFER_QUEUE) == static_cast(0x60), + "ApplyFlags operator |"); + +constexpr ApplyFlags operator&(ApplyFlags const& lhs, ApplyFlags const& rhs) { return static_cast( - static_cast(lhs) & - static_cast(rhs)); + static_cast>(lhs) & + static_cast>(rhs)); +} + +static_assert((tapPREFER_QUEUE & tapRETRY) == tapNONE, + "ApplyFlags operator &"); +static_assert((tapRETRY & tapPREFER_QUEUE) == tapNONE, + "ApplyFlags operator &"); + +constexpr +ApplyFlags +operator~(ApplyFlags const& flags) +{ + return static_cast( + ~static_cast>(flags)); +} + +static_assert(~tapRETRY == static_cast(~0x20), + "ApplyFlags operator ~"); + +inline +ApplyFlags +operator|=(ApplyFlags & lhs, + ApplyFlags const& rhs) +{ + lhs = lhs | rhs; + return lhs; +} + +inline +ApplyFlags +operator&=(ApplyFlags& lhs, + ApplyFlags const& rhs) +{ + lhs = lhs & rhs; + return lhs; } //------------------------------------------------------------------------------ diff --git a/src/test/app/LedgerHistory_test.cpp b/src/test/app/LedgerHistory_test.cpp index dc5574a3d..4d8560ef0 100644 --- a/src/test/app/LedgerHistory_test.cpp +++ b/src/test/app/LedgerHistory_test.cpp @@ -115,7 +115,7 @@ public: { OpenView accum(&*res); applyTransaction( - env.app(), accum, *stx, false, tapNO_CHECK_SIGN, env.journal); + env.app(), accum, *stx, false, tapNONE, env.journal); accum.apply(*res); } res->updateSkipList(); diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index 9bdac08b4..029eaa514 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -48,7 +48,7 @@ struct LedgerReplay_test : public beast::unit_test::suite auto const replayed = buildLedger( LedgerReplay(lastClosedParent,lastClosed), - tapNO_CHECK_SIGN, + tapNONE, env.app(), env.journal); diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 05cec4594..2c640fab5 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -344,6 +344,41 @@ public: 256); } + void testTecResult() + { + using namespace jtx; + + Env env(*this, + makeConfig({ { "minimum_txn_in_ledger_standalone", "2" } })); + + auto alice = Account("alice"); + auto gw = Account("gw"); + auto USD = gw["USD"]; + + checkMetrics(env, 0, boost::none, 0, 2, 256); + + // Create accounts + env.fund(XRP(50000), noripple(alice, gw)); + checkMetrics(env, 0, boost::none, 2, 2, 256); + env.close(); + checkMetrics(env, 0, 4, 0, 2, 256); + + // Alice creates an unfunded offer while the ledger is not full + env(offer(alice, XRP(1000), USD(1000)), ter(tecUNFUNDED_OFFER)); + checkMetrics(env, 0, 4, 1, 2, 256); + + fillQueue(env, alice); + checkMetrics(env, 0, 4, 3, 2, 256); + + // Alice creates an unfunded offer that goes in the queue + env(offer(alice, XRP(1000), USD(1000)), ter(terQUEUED)); + checkMetrics(env, 1, 4, 3, 2, 256); + + // The offer comes out of the queue + env.close(); + checkMetrics(env, 0, 6, 1, 3, 256); + } + void testLocalTxRetry() { using namespace jtx; diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index b47361992..5b1508b4b 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -363,25 +363,25 @@ class View_test BEAST_EXPECT(v1.parentCloseTime() == v1.parentCloseTime()); - ApplyViewImpl v2(&v1, tapNO_CHECK_SIGN); + ApplyViewImpl v2(&v1, tapRETRY); BEAST_EXPECT(v2.parentCloseTime() == v1.parentCloseTime()); BEAST_EXPECT(v2.seq() == v1.seq()); - BEAST_EXPECT(v2.flags() == tapNO_CHECK_SIGN); + BEAST_EXPECT(v2.flags() == tapRETRY); Sandbox v3(&v2); BEAST_EXPECT(v3.seq() == v2.seq()); BEAST_EXPECT(v3.parentCloseTime() == v2.parentCloseTime()); - BEAST_EXPECT(v3.flags() == tapNO_CHECK_SIGN); + BEAST_EXPECT(v3.flags() == tapRETRY); } { - ApplyViewImpl v1(&v0, tapNO_CHECK_SIGN); + ApplyViewImpl v1(&v0, tapRETRY); PaymentSandbox v2(&v1); BEAST_EXPECT(v2.seq() == v0.seq()); BEAST_EXPECT(v2.parentCloseTime() == v0.parentCloseTime()); - BEAST_EXPECT(v2.flags() == tapNO_CHECK_SIGN); + BEAST_EXPECT(v2.flags() == tapRETRY); PaymentSandbox v3(&v2); BEAST_EXPECT(v3.seq() == v2.seq()); BEAST_EXPECT(v3.parentCloseTime() ==