From c1b8efb7af094c3e4a531d957570062ec2453520 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 27 Jul 2016 20:31:04 -0400 Subject: [PATCH] Clear queue for account with high fee tx (RIPD-1246): * If an account has any transactions in the transaction queue, submitting a transaction that covers the differences to the open ledger fee level for prior queued transactions plus itself will cause all those transactions to be applied to the open ledger. * tel failures in `TxQ::accept` will leave tx in the queue to retry later. --- src/ripple/app/misc/TxQ.h | 47 ++++- src/ripple/app/misc/impl/TxQ.cpp | 256 +++++++++++++++++++---- src/ripple/app/tests/TxQ_test.cpp | 330 +++++++++++++++++++++++++----- 3 files changed, 548 insertions(+), 85 deletions(-) diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 03012ced51..7300156068 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -176,10 +176,10 @@ private: // before escalation kicks in. std::size_t txnsExpected_; // Minimum value of escalationMultiplier. - std::uint32_t const minimumMultiplier_; + std::uint64_t const minimumMultiplier_; // Based on the median fee of the LCL. Used // when fee escalation kicks in. - std::uint32_t escalationMultiplier_; + std::uint64_t escalationMultiplier_; beast::Journal j_; std::mutex mutable lock_; @@ -223,7 +223,7 @@ private: return txnsExpected_; } - std::uint32_t + std::uint64_t getEscalationMultiplier() const { std::lock_guard sl(lock_); @@ -233,6 +233,19 @@ private: std::uint64_t scaleFeeLevel(OpenView const& view, std::uint32_t txCountPadding = 0) const; + + /** + Returns the total fee level for all transactions in a series. + Assumes that there are already more than txnsExpected_ txns in + the view. If there aren't, the math still works, but the level + will be high. + + Returns: A `std::pair` as returned from `mulDiv` indicating whether + the calculation result is safe. + */ + std::pair + escalatedSeriesFeeLevel(OpenView const& view, std::size_t extraCount, + std::size_t seriesSize) const; }; class MaybeTx @@ -302,7 +315,19 @@ private: AccountID const account; // Sequence number will be used as the key. TxMap transactions; + /* If this account has had any transaction retry more than + `retriesAllowed` times so that it was dropped from the + queue, then all other transactions for this account will + be given at most 2 attempts before being removed. Helps + prevent wasting resources on retries that are more likely + to fail. + */ bool retryPenalty = false; + /* If this account has had any transaction fail or expire, + then when the queue is nearly full, transactions from + this account will be discarded. Helps prevent the queue + from getting filled and wedged. + */ bool dropPenalty = false; public: @@ -365,6 +390,22 @@ private: // is higher), or next entry in byFee_ (lower fee level). // Used to get the next "applyable" MaybeTx for accept(). FeeMultiSet::iterator_type eraseAndAdvance(FeeMultiSet::const_iterator_type); + // Erase a range of items, based on TxQAccount::TxMap iterators + TxQAccount::TxMap::iterator + erase(TxQAccount& txQAccount, TxQAccount::TxMap::const_iterator begin, + TxQAccount::TxMap::const_iterator end); + + /* + All-or-nothing attempt to try to apply all the queued txs for `accountIter` + up to and including `tx`. + */ + std::pair + tryClearAccountQueue(Application& app, OpenView& view, + STTx const& tx, AccountMap::iterator const& accountIter, + TxQAccount::TxMap::iterator, std::uint64_t feeLevelPaid, + PreflightResult const& pfresult, + std::size_t const txExtraCount, ApplyFlags flags, + beast::Journal j); }; diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index d0bd544ed5..2551f099b8 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -81,17 +81,17 @@ TxQ::FeeMetrics::update(Application& app, ReadView const& view, bool timeLeap, TxQ::Setup const& setup) { - std::vector feeLevels; std::size_t txnsExpected; std::size_t mimimumTx; - std::uint32_t escalationMultiplier; + std::uint64_t escalationMultiplier; { std::lock_guard sl(lock_); - feeLevels.reserve(txnsExpected_); txnsExpected = txnsExpected_; mimimumTx = minimumTxnCount_; escalationMultiplier = escalationMultiplier_; } + std::vector feeLevels; + feeLevels.reserve(txnsExpected); for (auto const& tx : view.txs) { auto const baseFee = calculateBaseFee(app, view, @@ -161,15 +161,14 @@ TxQ::FeeMetrics::scaleFeeLevel(OpenView const& view, // Transactions in the open ledger so far auto const current = view.txCount() + txCountPadding; - std::size_t target; - std::uint32_t multiplier; + auto const params = [&] { std::lock_guard sl(lock_); - - // Target number of transactions allowed - target = txnsExpected_; - multiplier = escalationMultiplier_; - } + return std::make_pair(txnsExpected_, + escalationMultiplier_); + }(); + auto const target = params.first; + auto const multiplier = params.second; // Once the open ledger bypasses the target, // escalate the fee quickly. @@ -184,6 +183,71 @@ TxQ::FeeMetrics::scaleFeeLevel(OpenView const& view, return baseLevel; } +namespace detail { + +static +std::pair +sumOfFirstSquares(std::size_t x) +{ + // sum(n = 1->x) : n * n = x(x + 1)(2x + 1) / 6 + + // If x is anywhere on the order of 2^^21, it's going + // to completely dominate the computation and is likely + // enough to overflow that we're just going to assume + // it does. If we have anywhere near 2^^21 transactions + // in a ledger, this is the least of our problems. + if (x >= (1 << 21)) + return std::make_pair(false, + std::numeric_limits::max()); + return std::make_pair(true, (x * (x + 1) * (2 * x + 1)) / 6); +} + +} + +std::pair +TxQ::FeeMetrics::escalatedSeriesFeeLevel(OpenView const& view, + std::size_t extraCount, std::size_t seriesSize) const +{ + /* Transactions in the open ledger so far. + AKA Transactions that will be in the open ledger when + the first tx in the series is attempted. + */ + auto const current = view.txCount() + extraCount; + /* Transactions that will be in the open ledger when + the last tx in the series is attempted. + */ + auto const last = current + seriesSize - 1; + + auto const params = [&] { + std::lock_guard sl(lock_); + return std::make_pair(txnsExpected_, + escalationMultiplier_); + }(); + auto const target = params.first; + auto const multiplier = params.second; + + assert(current > target); + + /* Calculate (apologies for the terrible notation) + sum(n = current -> last) : multiplier * n * n / (target * target) + multiplier / (target * target) * (sum(n = current -> last) : n * n) + multiplier / (target * target) * ((sum(n = 1 -> last) : n * n) - + (sum(n = 1 -> current - 1) : n * n)) + */ + auto const sumNlast = detail::sumOfFirstSquares(last); + auto const sumNcurrent = detail::sumOfFirstSquares(current - 1); + // because `last` is bigger, if either sum overflowed, then + // `sumNlast` definitely overflowed. Also the odds of this + // are nearly nil. + if (!sumNlast.first) + return sumNlast; + auto const totalFeeLevel = mulDiv(multiplier, + sumNlast.second - sumNcurrent.second, target * target); + + return totalFeeLevel; +} + + TxQ::MaybeTx::MaybeTx( std::shared_ptr const& txn_, TxID const& txID_, std::uint64_t feeLevel_, @@ -321,12 +385,12 @@ TxQ::erase(TxQ::FeeMultiSet::const_iterator_type candidateIter) -> FeeMultiSet::iterator_type { auto& txQAccount = byAccount_.at(candidateIter->account); - auto sequence = candidateIter->sequence; - auto newCandidateIter = byFee_.erase(candidateIter); + auto const sequence = candidateIter->sequence; + auto const newCandidateIter = byFee_.erase(candidateIter); // Now that the candidate has been removed from the // intrusive list remove it from the TxQAccount // so the memory can be freed. - auto found = txQAccount.remove(sequence); + auto const found = txQAccount.remove(sequence); (void)found; assert(found); @@ -338,16 +402,16 @@ TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter) -> FeeMultiSet::iterator_type { auto& txQAccount = byAccount_.at(candidateIter->account); - auto accountIter = txQAccount.transactions.find( + auto const accountIter = txQAccount.transactions.find( candidateIter->sequence); assert(accountIter != txQAccount.transactions.end()); assert(accountIter == txQAccount.transactions.begin()); assert(byFee_.iterator_to(accountIter->second) == candidateIter); - auto accountNextIter = std::next(accountIter); + auto const accountNextIter = std::next(accountIter); /* Check if the next transaction for this account has the next sequence number, and a higher fee level, which means we skipped it earlier, and need to try it again. - Edge cases: If the next acccount tx has a lower fee level, + Edge cases: If the next account tx has a lower fee level, it's going to be later in the fee queue, so we haven't skipped it yet. If the next tx has an equal fee level, it was either @@ -357,12 +421,12 @@ TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter) the latter case, continue through the fee queue anyway to head off potential ordering manipulation problems. */ - auto feeNextIter = std::next(candidateIter); - bool useAccountNext = accountNextIter != txQAccount.transactions.end() && + auto const feeNextIter = std::next(candidateIter); + bool const useAccountNext = accountNextIter != txQAccount.transactions.end() && accountNextIter->first == candidateIter->sequence + 1 && (feeNextIter == byFee_.end() || accountNextIter->second.feeLevel > feeNextIter->feeLevel); - auto candidateNextIter = byFee_.erase(candidateIter); + auto const candidateNextIter = byFee_.erase(candidateIter); txQAccount.transactions.erase(accountIter); return useAccountNext ? byFee_.iterator_to(accountNextIter->second) : @@ -370,6 +434,89 @@ TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter) } +auto +TxQ::erase(TxQ::TxQAccount& txQAccount, + TxQ::TxQAccount::TxMap::const_iterator begin, + TxQ::TxQAccount::TxMap::const_iterator end) + -> TxQAccount::TxMap::iterator +{ + for (auto it = begin; it != end; ++it) + { + byFee_.erase(byFee_.iterator_to(it->second)); + } + return txQAccount.transactions.erase(begin, end); +} + +std::pair +TxQ::tryClearAccountQueue(Application& app, OpenView& view, + STTx const& tx, TxQ::AccountMap::iterator const& accountIter, + TxQAccount::TxMap::iterator beginTxIter, std::uint64_t feeLevelPaid, + PreflightResult const& pfresult, std::size_t const txExtraCount, + ApplyFlags flags, beast::Journal j) +{ + auto const tSeq = tx.getSequence(); + assert(beginTxIter != accountIter->second.transactions.end()); + auto const aSeq = beginTxIter->first; + + auto const requiredTotalFeeLevel = feeMetrics_.escalatedSeriesFeeLevel( + view, txExtraCount, tSeq - aSeq + 1); + /* If the computation for the total manages to overflow (however extremely + unlikely), then there's no way we can confidently verify if the queue + can be cleared. + */ + if (!requiredTotalFeeLevel.first) + return std::make_pair(telINSUF_FEE_P, false); + + // Unlike multiTx, this check is only concerned with the range + // from [aSeq, tSeq) + auto endTxIter = accountIter->second.transactions.lower_bound(tSeq); + + auto const totalFeeLevelPaid = std::accumulate(beginTxIter, endTxIter, + feeLevelPaid, + [](auto const& total, auto const& tx) + { + return total + tx.second.feeLevel; + }); + + // This transaction did not pay enough, so fall back to the normal process. + if (totalFeeLevelPaid < requiredTotalFeeLevel.second) + return std::make_pair(telINSUF_FEE_P, false); + + // This transaction paid enough to clear out the queue. + // Attempt to apply the queued transactions. + for (auto it = beginTxIter; it != endTxIter; ++it) + { + auto txResult = it->second.apply(app, view); + // 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 + // moot. + --it->second.retriesRemaining; + if (!txResult.second) + { + // Transaction failed to apply. Fall back to the normal process. + return std::make_pair(txResult.first, false); + } + } + // Apply the current tx. Because the state of the view has been changed + // by the queued txs, we also need to preclaim again. + auto const pcresult = preclaim(pfresult, app, view); + auto txResult = doApply(pcresult, app, view); + + if (txResult.second) + { + // All of the queued transactions applied, so remove them from the queue. + endTxIter = erase(accountIter->second, beginTxIter, endTxIter); + // If `tx` is replacing a queued tx, delete that one, too. + if (endTxIter != accountIter->second.transactions.end() && + endTxIter->first == tSeq) + erase(accountIter->second, endTxIter, std::next(endTxIter)); + } + + return txResult; +} + + /* How the decision to apply, queue, or reject is made: 0. Is `featureFeeEscalation` enabled? @@ -451,7 +598,7 @@ TxQ::apply(Application& app, OpenView& view, auto const account = (*tx)[sfAccount]; auto const transactionID = tx->getTransactionID(); - auto const t_seq = tx->getSequence(); + auto const tSeq = tx->getSequence(); // See if the transaction is valid, properly formed, // etc. before doing potentially expensive queue @@ -466,6 +613,8 @@ TxQ::apply(Application& app, OpenView& view, boost::optional applyView; boost::optional openView; + TxQAccount::TxMap::iterator nextTxIter; + XRPAmount fee = beast::zero; XRPAmount potentialSpend = beast::zero; bool includeCurrentFee = false; @@ -494,7 +643,7 @@ TxQ::apply(Application& app, OpenView& view, if (accountExists) { auto& txQAcct = accountIter->second; - auto existingIter = txQAcct.transactions.find(t_seq); + auto existingIter = txQAcct.transactions.find(tSeq); if (existingIter != txQAcct.transactions.end()) { // Is the current transaction's fee higher than @@ -503,7 +652,7 @@ TxQ::apply(Application& app, OpenView& view, existingIter->second.feeLevel, setup_.retrySequencePercent); JLOG(j_.trace()) << "Found transaction in queue for account " << - account << " with sequence number " << t_seq << + account << " with sequence number " << tSeq << " new txn fee level is " << feeLevelPaid << ", old txn fee level is " << existingIter->second.feeLevel << @@ -568,7 +717,7 @@ TxQ::apply(Application& app, OpenView& view, auto deleteIter = byFee_.iterator_to(existingIter->second); assert(deleteIter != byFee_.end()); assert(&existingIter->second == &*deleteIter); - assert(deleteIter->sequence == t_seq); + assert(deleteIter->sequence == tSeq); assert(deleteIter->account == txQAcct.account); replacedItemDeleteIter = deleteIter; } @@ -595,9 +744,9 @@ TxQ::apply(Application& app, OpenView& view, if (sle) { auto& txQAcct = accountIter->second; - auto const a_seq = (*sle)[sfSequence]; + auto const aSeq = (*sle)[sfSequence]; - if (a_seq < t_seq) + if (aSeq < tSeq) { // If the transaction is queueable, create the multiTxn // object to hold the info we need to adjust for @@ -610,23 +759,24 @@ TxQ::apply(Application& app, OpenView& view, if (multiTxn) { /* See if the queue has entries for all the - seq's in [a_seq, t_seq). Total up all the + seq's in [aSeq, tSeq). Total up all the consequences while we're checking. If one turns up missing or is a blocker, abort. */ - auto workingIter = txQAcct.transactions.find(a_seq); - auto workingSeq = a_seq; + multiTxn->nextTxIter = txQAcct.transactions.find(aSeq); + auto workingIter = multiTxn->nextTxIter; + auto workingSeq = aSeq; for (; workingIter != txQAcct.transactions.end(); ++workingIter, ++workingSeq) { - if (workingSeq < t_seq && + if (workingSeq < tSeq && workingIter->first != workingSeq) { // If any transactions are missing before `tx`, abort. multiTxn.reset(); break; } - if (workingIter->first == t_seq - 1) + if (workingIter->first == tSeq - 1) { // Is the current transaction's fee higher than // the previous transaction's fee + a percentage @@ -648,7 +798,7 @@ TxQ::apply(Application& app, OpenView& view, return{ telINSUF_FEE_P, false }; } } - if (workingIter->first == t_seq) + if (workingIter->first == tSeq) { // If we're replacing this transaction, don't // count it. @@ -664,7 +814,7 @@ TxQ::apply(Application& app, OpenView& view, *workingIter->second.pfresult)); // Don't worry about the blocker status of txs // after the current. - if (workingIter->first < t_seq && + if (workingIter->first < tSeq && workingIter->second.consequences->category == TxConsequences::blocker) { @@ -682,7 +832,7 @@ TxQ::apply(Application& app, OpenView& view, multiTxn->potentialSpend += workingIter->second.consequences->potentialSpend; } - if (workingSeq < t_seq) + if (workingSeq < tSeq) // Transactions are missing before `tx`. multiTxn.reset(); } @@ -748,7 +898,7 @@ TxQ::apply(Application& app, OpenView& view, sleBump->setFieldAmount(sfBalance, balance - (multiTxn->fee + multiTxn->potentialSpend)); - sleBump->setFieldU32(sfSequence, t_seq); + sleBump->setFieldU32(sfSequence, tSeq); } } } @@ -771,6 +921,42 @@ TxQ::apply(Application& app, OpenView& view, " to get in the open ledger, which has " << view.txCount() << " entries."; + /* Quick heuristic check to see if it's worth checking that this + tx has a high enough fee to clear all the txs in the queue. + 1) Must be an account already in the queue. + 2) Must be have passed the multiTxn checks (tx is not the next + account seq, the skipped seqs are in the queue, the reserve + doesn't get exhausted, etc). + 3) The next transaction must not have previously tried and failed + to apply to an open ledger. + 4) Tx must be paying more than just the required fee level to + get itself into the queue. + 5) Fee level must be escalated above the default (if it's not, + then the first tx _must_ have failed to process in `accept` + for some other reason. Tx is allowed to queue in case + conditions change, but don't waste the effort to clear). + 6) Tx is not a 0-fee / free transaction, regardless of fee level. + */ + if (accountExists && multiTxn.is_initialized() && + multiTxn->nextTxIter->second.retriesRemaining == MaybeTx::retriesAllowed && + feeLevelPaid > requiredFeeLevel && + requiredFeeLevel > baseLevel && baseFee != 0) + { + OpenView sandbox(open_ledger, &view, view.rules()); + + auto result = tryClearAccountQueue(app, sandbox, *tx, accountIter, + multiTxn->nextTxIter, feeLevelPaid, pfresult, view.txCount(), + flags, j); + if (result.second) + { + sandbox.apply(view); + /* Can't erase(*replacedItemDeleteIter) here because success + implies that it has already been deleted. + */ + return result; + } + } + // Can transaction go in open ledger? if (!multiTxn && feeLevelPaid >= requiredFeeLevel) { @@ -1064,7 +1250,7 @@ TxQ::accept(Application& app, ledgerChanged = true; } else if (isTefFailure(txnResult) || isTemMalformed(txnResult) || - isTelLocal(txnResult) || candidateIter->retriesRemaining <= 0) + candidateIter->retriesRemaining <= 0) { if (candidateIter->retriesRemaining <= 0) account.retryPenalty = true; diff --git a/src/ripple/app/tests/TxQ_test.cpp b/src/ripple/app/tests/TxQ_test.cpp index a6d5e0b205..b5e5d46ec5 100644 --- a/src/ripple/app/tests/TxQ_test.cpp +++ b/src/ripple/app/tests/TxQ_test.cpp @@ -28,9 +28,10 @@ #include #include #include -#include +#include #include #include +#include namespace ripple { namespace test { @@ -112,6 +113,48 @@ class TxQ_test : public beast::unit_test::suite return p; } + void + initFee(jtx::Env& env, std::size_t expectedInLedger, std::uint32_t base, + std::uint32_t units, std::uint32_t reserve, std::uint32_t increment) + { + // Change the reserve fee so we can create an account + // with a lower balance. + STTx feeTx(ttFEE, + [&](auto& obj) + { + obj[sfAccount] = AccountID(); + obj[sfLedgerSequence] = env.current()->info().seq; + obj[sfBaseFee] = base; + obj[sfReferenceFeeUnits] = units; + obj[sfReserveBase] = reserve; + obj[sfReserveIncrement] = increment; + }); + + auto const changed = env.app().openLedger().modify( + [&](OpenView& view, beast::Journal j) + { + auto const result = ripple::apply( + env.app(), view, feeTx, + tapNONE, j); + BEAST_EXPECT(result.second && result.first == + tesSUCCESS); + return result.second; + }); + BEAST_EXPECT(changed); + // Pad a couple of txs to keep the median at the default + env(noop(env.master)); + env(noop(env.master)); + // Close the ledger with a delay to force the TxQ stats + // to stay at the default. + env.close(env.now() + 5s, 10000ms); + checkMetrics(env, 0, boost::none, 0, expectedInLedger, 256); + auto const fees = env.current()->fees(); + BEAST_EXPECT(fees.base == base); + BEAST_EXPECT(fees.units == units); + BEAST_EXPECT(fees.reserve == reserve); + BEAST_EXPECT(fees.increment == increment); + } + public: void testQueue() { @@ -676,8 +719,11 @@ public: checkMetrics(env, 0, boost::none, 0, 3, 256); + initFee(env, 3, 10, 10, 200, 50); + // Create several accounts while the fee is cheap so they all apply. - env.fund(XRP(50000), noripple(alice, bob, charlie, daria)); + env.fund(drops(2000), noripple(alice)); + env.fund(XRP(500000), noripple(bob, charlie, daria)); checkMetrics(env, 0, boost::none, 4, 3, 256); // Alice - price starts exploding: held @@ -734,7 +780,7 @@ public: BEAST_EXPECT(env.seq(charlie) == charlieSeq + 1); // Alice - fill up the queue - std::int64_t aliceFee = 10; + std::int64_t aliceFee = 20; aliceSeq = env.seq(alice); auto lastLedgerSeq = env.current()->info().seq + 2; for (auto i = 0; i < 7; i++) @@ -742,15 +788,14 @@ public: env(noop(alice), seq(aliceSeq), json(jss::LastLedgerSequence, lastLedgerSeq + i), fee(aliceFee), queued); - aliceFee = (aliceFee + 1) * 125 / 100; ++aliceSeq; } - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); { auto& txQ = env.app().getTxQ(); auto aliceStat = txQ.getAccountTxs(alice.id(), env.app().config(), *env.current()); - std::int64_t fee = 10; + std::int64_t fee = 20; auto seq = env.seq(alice); if (BEAST_EXPECT(aliceStat)) { @@ -766,7 +811,6 @@ public: tx.second.consequences->category == TxConsequences::normal) || tx.first == env.seq(alice) + 6); ++seq; - fee = (fee + 1) * 125 / 100; } } } @@ -777,60 +821,46 @@ public: env(noop(alice), seq(aliceSeq), json(jss::LastLedgerSequence, lastLedgerSeq + 7), fee(aliceFee), ter(telCAN_NOT_QUEUE)); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); // Charlie - try to add another item to the queue, // which fails because fee is lower than Alice's // queued average. - env(noop(charlie), fee(24), ter(telINSUF_FEE_P)); - checkMetrics(env, 8, 8, 5, 4, 257); + env(noop(charlie), fee(19), ter(telINSUF_FEE_P)); + checkMetrics(env, 8, 8, 5, 4, 513); // Charlie - add another item to the queue, which // causes Alice's last txn to drop env(noop(charlie), fee(30), queued); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); // Alice - now attempt to add one more to the queue, // which fails because the last tx was dropped, so // there is no complete chain. env(noop(alice), seq(aliceSeq), fee(aliceFee), ter(terPRE_SEQ)); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); // Alice wants this tx more than the dropped tx, // so resubmits with higher fee, but the queue // is full, and her account is the cheapest. env(noop(alice), seq(aliceSeq - 1), fee(aliceFee), ter(telCAN_NOT_QUEUE)); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); // Try to replace a middle item in the queue // without enough fee. aliceSeq = env.seq(alice) + 2; - aliceFee = 21; + aliceFee = 25; env(noop(alice), seq(aliceSeq), fee(aliceFee), ter(telINSUF_FEE_P)); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); // Replace a middle item from the queue successfully ++aliceFee; env(noop(alice), seq(aliceSeq), fee(aliceFee), queued); - checkMetrics(env, 8, 8, 5, 4, 257); - - // Try to replace the next item in the queue - // without enough fee. - ++aliceSeq; - aliceFee = aliceFee * 125 / 100; - env(noop(alice), seq(aliceSeq), - fee(aliceFee), ter(telINSUF_FEE_P)); - checkMetrics(env, 8, 8, 5, 4, 257); - - // Replace a middle item from the queue successfully - ++aliceFee; - env(noop(alice), seq(aliceSeq), - fee(aliceFee), queued); - checkMetrics(env, 8, 8, 5, 4, 257); + checkMetrics(env, 8, 8, 5, 4, 513); env.close(); // Alice's transactions processed, along with @@ -845,7 +875,7 @@ public: // more than the minimum reserve in flight before the // last queued transaction aliceFee = env.le(alice)->getFieldAmount(sfBalance).xrp().drops() - - (121); + - (59); env(noop(alice), seq(aliceSeq), fee(aliceFee), ter(telCAN_NOT_QUEUE)); checkMetrics(env, 4, 10, 6, 5, 256); @@ -1144,15 +1174,18 @@ public: auto queued = ter(terQUEUED); + initFee(env, 3, 10, 10, 200, 50); + BEAST_EXPECT(env.current()->fees().base == 10); checkMetrics(env, 0, boost::none, 0, 3, 256); - env.fund(XRP(50000), noripple(alice, bob)); + env.fund(drops(5000), noripple(alice)); + env.fund(XRP(50000), noripple(bob)); checkMetrics(env, 0, boost::none, 2, 3, 256); auto USD = bob["USD"]; - env(offer(alice, USD(5000), XRP(50000)), require(owners(alice, 1))); + env(offer(alice, USD(5000), drops(5000)), require(owners(alice, 1))); checkMetrics(env, 0, boost::none, 3, 3, 256); env.close(); @@ -1163,25 +1196,25 @@ public: checkMetrics(env, 0, 6, 4, 3, 256); // Queue up a couple of transactions, plus one - // really expensive one. + // more expensive one. auto aliceSeq = env.seq(alice); env(noop(alice), seq(aliceSeq++), queued); env(noop(alice), seq(aliceSeq++), queued); env(noop(alice), seq(aliceSeq++), queued); - env(noop(alice), fee(XRP(1000)), + env(noop(alice), fee(drops(1000)), seq(aliceSeq), queued); checkMetrics(env, 4, 6, 4, 3, 256); // This offer should take Alice's offer // up to Alice's reserve. - env(offer(bob, XRP(50000), USD(5000)), - openLedgerFee(env), require(balance("alice", XRP(250)), + env(offer(bob, drops(5000), USD(5000)), + openLedgerFee(env), require(balance(alice, drops(250)), owners(alice, 1), lines(alice, 1))); checkMetrics(env, 4, 6, 5, 3, 256); // Try adding a new transaction. // Too many fees in flight. - env(noop(alice), fee(XRP(2000)), seq(aliceSeq+1), + env(noop(alice), fee(drops(200)), seq(aliceSeq+1), ter(telCAN_NOT_QUEUE)); checkMetrics(env, 4, 6, 5, 3, 256); @@ -1189,11 +1222,11 @@ public: // take a fee, except the last one. env.close(); checkMetrics(env, 1, 10, 3, 5, 256); - env.require(balance(alice, XRP(250) - drops(30))); + env.require(balance(alice, drops(250 - 30))); // Still can't add a new transaction for Alice, // no matter the fee. - env(noop(alice), fee(XRP(2000)), seq(aliceSeq + 1), + env(noop(alice), fee(drops(200)), seq(aliceSeq + 1), ter(telCAN_NOT_QUEUE)); checkMetrics(env, 1, 10, 3, 5, 256); @@ -1671,8 +1704,8 @@ public: void testExpirationReplacement() { /* This test is based on a reported regression where a - replacement candidate transaction found it's replacee - did not have `consequences` set + replacement candidate transaction found the tx it was trying + to replace did not have `consequences` set Hypothesis: The queue had '22 through '25. At some point(s), both the original '22 and '23 expired and were removed from @@ -2202,7 +2235,6 @@ public: } { auto const server_state = env.rpc("server_state"); - log << server_state; auto const& state = server_state[jss::result][jss::state]; BEAST_EXPECT(state.isMember(jss::load_factor) && state[jss::load_factor] == 256); @@ -2248,7 +2280,6 @@ public: } { auto const server_state = env.rpc("server_state"); - log << server_state; auto const& state = server_state[jss::result][jss::state]; BEAST_EXPECT(state.isMember(jss::load_factor) && state[jss::load_factor] == 227555); @@ -2284,7 +2315,6 @@ public: } { auto const server_state = env.rpc("server_state"); - log << server_state; auto const& state = server_state[jss::result][jss::state]; BEAST_EXPECT(state.isMember(jss::load_factor) && state[jss::load_factor] == 256000); @@ -2332,7 +2362,6 @@ public: } { auto const server_state = env.rpc("server_state"); - log << server_state; auto const& state = server_state[jss::result][jss::state]; BEAST_EXPECT(state.isMember(jss::load_factor) && state[jss::load_factor] == 227555); @@ -2376,7 +2405,6 @@ public: } { auto const server_state = env.rpc("server_state"); - log << server_state; auto const& state = server_state[jss::result][jss::state]; BEAST_EXPECT(state.isMember(jss::load_factor) && state[jss::load_factor] >= 320 && @@ -2398,6 +2426,213 @@ public: } } + void testClearQueuedAccountTxs() + { + using namespace jtx; + + Env env(*this, + makeConfig({ { "minimum_txn_in_ledger_standalone", "3" } }), + features(featureFeeEscalation)); + auto alice = Account("alice"); + auto bob = Account("bob"); + + checkMetrics(env, 0, boost::none, 0, 3, 256); + env.fund(XRP(50000000), alice, bob); + + fillQueue(env, alice); + + auto calcTotalFee = [&]( + std::int64_t alreadyPaid, boost::optional numToClear = boost::none) + -> std::uint64_t { + auto totalFactor = 0; + auto const metrics = env.app ().getTxQ ().getMetrics ( + env.app ().config (), *env.current ()); + if (!numToClear) + numToClear.emplace(metrics->txCount + 1); + for (int i = 0; i < *numToClear; ++i) + { + auto inLedger = metrics->txInLedger + i; + totalFactor += inLedger * inLedger; + } + auto result = + mulDiv (metrics->medFeeLevel * totalFactor / + (metrics->txPerLedger * metrics->txPerLedger), + env.current ()->fees ().base, metrics->referenceFeeLevel) + .second; + // Subtract the fees already paid + result -= alreadyPaid; + // round up + ++result; + return result; + }; + + testcase("straightfoward positive case"); + { + // Queue up some transactions at a too-low fee. + auto aliceSeq = env.seq(alice); + for (int i = 0; i < 2; ++i) + { + env(noop(alice), fee(100), seq(aliceSeq++), ter(terQUEUED)); + } + + // Queue up a transaction paying the open ledger fee + // This will be the first tx to call the operative function, + // but it won't succeed. + env(noop(alice), openLedgerFee(env), seq(aliceSeq++), + ter(terQUEUED)); + + checkMetrics(env, 3, boost::none, 4, 3, 256); + + // Figure out how much it would cost to cover all the + // queued txs + itself + std::uint64_t totalFee1 = calcTotalFee (100 * 2 + 8889); + --totalFee1; + + BEAST_EXPECT(totalFee1 == 60911); + // Submit a transaction with that fee. It will get queued + // because the fee level calculation rounds down. This is + // the edge case test. + env(noop(alice), fee(totalFee1), seq(aliceSeq++), + ter(terQUEUED)); + + checkMetrics(env, 4, boost::none, 4, 3, 256); + + // Now repeat the process including the new tx + // and avoiding the rounding error + std::uint64_t const totalFee2 = calcTotalFee (100 * 2 + 8889 + 60911); + BEAST_EXPECT(totalFee2 == 35556); + // Submit a transaction with that fee. It will succeed. + env(noop(alice), fee(totalFee2), seq(aliceSeq++)); + + checkMetrics(env, 0, boost::none, 9, 3, 256); + } + + testcase("replace last tx with enough to clear queue"); + { + // Queue up some transactions at a too-low fee. + auto aliceSeq = env.seq(alice); + for (int i = 0; i < 2; ++i) + { + env(noop(alice), fee(100), seq(aliceSeq++), ter(terQUEUED)); + } + + // Queue up a transaction paying the open ledger fee + // This will be the first tx to call the operative function, + // but it won't succeed. + env(noop(alice), openLedgerFee(env), seq(aliceSeq++), + ter(terQUEUED)); + + checkMetrics(env, 3, boost::none, 9, 3, 256); + + // Figure out how much it would cost to cover all the + // queued txs + itself + auto const metrics = env.app ().getTxQ ().getMetrics ( + env.app ().config (), *env.current ()); + std::uint64_t const totalFee = + calcTotalFee (100 * 2, metrics->txCount); + BEAST_EXPECT(totalFee == 167578); + // Replacing the last tx with the large fee succeeds. + --aliceSeq; + env(noop(alice), fee(totalFee), seq(aliceSeq++)); + + // The queue is clear + checkMetrics(env, 0, boost::none, 12, 3, 256); + + env.close(); + checkMetrics(env, 0, 24, 0, 12, 256); + } + + testcase("replace middle tx with enough to clear queue"); + { + fillQueue(env, alice); + // Queue up some transactions at a too-low fee. + auto aliceSeq = env.seq(alice); + for (int i = 0; i < 5; ++i) + { + env(noop(alice), fee(100), seq(aliceSeq++), ter(terQUEUED)); + } + + checkMetrics(env, 5, 24, 13, 12, 256); + + // Figure out how much it would cost to cover 3 txns + std::uint64_t const totalFee = calcTotalFee(100 * 2, 3); + BEAST_EXPECT(totalFee == 20287); + // Replacing the last tx with the large fee succeeds. + aliceSeq -= 3; + env(noop(alice), fee(totalFee), seq(aliceSeq++)); + + checkMetrics(env, 2, 24, 16, 12, 256); + auto const aliceQueue = env.app().getTxQ().getAccountTxs( + alice.id(), env.app().config(), *env.current()); + if (BEAST_EXPECT(aliceQueue)) + { + BEAST_EXPECT(aliceQueue->size() == 2); + auto seq = aliceSeq; + for (auto const& tx : *aliceQueue) + { + BEAST_EXPECT(tx.first == seq); + BEAST_EXPECT(tx.second.feeLevel == 2560); + ++seq; + } + } + + // Close the ledger to clear the queue + env.close(); + checkMetrics(env, 0, 32, 2, 16, 256); + } + + testcase("clear queue failure (load)"); + { + fillQueue(env, alice); + // Queue up some transactions at a too-low fee. + auto aliceSeq = env.seq(alice); + for (int i = 0; i < 2; ++i) + { + env(noop(alice), fee(200), seq(aliceSeq++), ter(terQUEUED)); + } + for (int i = 0; i < 2; ++i) + { + env(noop(alice), fee(22), seq(aliceSeq++), ter(terQUEUED)); + } + + checkMetrics(env, 4, 32, 17, 16, 256); + + // Figure out how much it would cost to cover all the txns + // + 1 + std::uint64_t const totalFee = calcTotalFee (200 * 2 + 22 * 2); + BEAST_EXPECT(totalFee == 35006); + // This fee should be enough, but oh no! Server load went up! + auto& feeTrack = env.app().getFeeTrack(); + auto const origFee = feeTrack.getRemoteFee(); + feeTrack.setRemoteFee(origFee * 5); + // Instead the tx gets queued, and all of the queued + // txs stay in the queue. + env(noop(alice), fee(totalFee), seq(aliceSeq++), ter(terQUEUED)); + + // The original last transaction is still in the queue + checkMetrics(env, 5, 32, 17, 16, 256); + + // With high load, some of the txs stay in the queue + env.close(); + checkMetrics(env, 3, 34, 2, 17, 256); + + // Load drops back down + feeTrack.setRemoteFee(origFee); + + // Because of the earlier failure, alice can not clear the queue, + // no matter how high the fee + fillQueue(env, bob); + checkMetrics(env, 3, 34, 18, 17, 256); + + env(noop(alice), fee(XRP(1)), seq(aliceSeq++), ter(terQUEUED)); + checkMetrics(env, 4, 34, 18, 17, 256); + + // With normal load, those txs get into the ledger + env.close(); + checkMetrics(env, 0, 36, 4, 18, 256); + } + } + void run() { testQueue(); @@ -2420,6 +2655,7 @@ public: testSignAndSubmitSequence(); testAccountInfo(); testServerInfo(); + testClearQueuedAccountTxs(); } };