From d6fdd4c9e0f4d3c859561919b4c2e52245ca51a7 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 1 Dec 2021 19:23:58 -0500 Subject: [PATCH 1/5] Logging & minor optimizations: * Log load fee values (at debug) received from validations. * Log remote and cluster fee values (at trace) when changed. * Refactor JobQueue::isOverloaded to return sooner if overloaded. * Refactor Transactor::checkFee to only compute fee if ledger is open. --- src/ripple/app/ledger/impl/LedgerMaster.cpp | 10 ++++++++++ src/ripple/app/misc/LoadFeeTrack.h | 3 +++ src/ripple/app/tx/impl/Transactor.cpp | 18 +++++++++++------- src/ripple/core/impl/JobQueue.cpp | 6 ++---- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index 7bc1bf243cf..4cc19581543 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -1080,6 +1080,16 @@ LedgerMaster::checkAccept(std::shared_ptr const& ledger) if (!fees.empty()) { std::sort(fees.begin(), fees.end()); + if (auto stream = m_journal.debug()) + { + std::stringstream s; + s << "Received fees from validations: (" << fees.size() << ") "; + for (auto const fee1 : fees) + { + s << " " << fee1; + } + stream << s.str(); + } fee = fees[fees.size() / 2]; // median } else diff --git a/src/ripple/app/misc/LoadFeeTrack.h b/src/ripple/app/misc/LoadFeeTrack.h index 30c8766a9b3..0109468cb99 100644 --- a/src/ripple/app/misc/LoadFeeTrack.h +++ b/src/ripple/app/misc/LoadFeeTrack.h @@ -21,6 +21,7 @@ #define RIPPLE_CORE_LOADFEETRACK_H_INCLUDED #include +#include #include #include #include @@ -58,6 +59,7 @@ class LoadFeeTrack final void setRemoteFee(std::uint32_t f) { + JLOG(j_.trace()) << "setRemoteFee: " << f; std::lock_guard sl(lock_); remoteTxnLoadFee_ = f; } @@ -110,6 +112,7 @@ class LoadFeeTrack final void setClusterFee(std::uint32_t fee) { + JLOG(j_.trace()) << "setClusterFee: " << fee; std::lock_guard sl(lock_); clusterTxnLoadFee_ = fee; } diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 8b74463de56..619ce031b82 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -174,15 +174,19 @@ Transactor::checkFee(PreclaimContext const& ctx, FeeUnit64 baseFee) if (!isLegalAmount(feePaid) || feePaid < beast::zero) return temBAD_FEE; - auto const feeDue = - minimumFee(ctx.app, baseFee, ctx.view.fees(), ctx.flags); - // Only check fee is sufficient when the ledger is open. - if (ctx.view.open() && feePaid < feeDue) + if (ctx.view.open()) { - JLOG(ctx.j.trace()) << "Insufficient fee paid: " << to_string(feePaid) - << "/" << to_string(feeDue); - return telINSUF_FEE_P; + auto const feeDue = + minimumFee(ctx.app, baseFee, ctx.view.fees(), ctx.flags); + + if (feePaid < feeDue) + { + JLOG(ctx.j.trace()) + << "Insufficient fee paid: " << to_string(feePaid) << "/" + << to_string(feeDue); + return telINSUF_FEE_P; + } } if (feePaid == beast::zero) diff --git a/src/ripple/core/impl/JobQueue.cpp b/src/ripple/core/impl/JobQueue.cpp index d87664416a0..fc50ff603e4 100644 --- a/src/ripple/core/impl/JobQueue.cpp +++ b/src/ripple/core/impl/JobQueue.cpp @@ -168,15 +168,13 @@ JobQueue::addLoadEvents(JobType t, int count, std::chrono::milliseconds elapsed) bool JobQueue::isOverloaded() { - int count = 0; - for (auto& x : m_jobData) { if (x.second.load().isOver()) - ++count; + return true; } - return count > 0; + return false; } Json::Value From ef635e7f20ef8c2a70a308b14f913d87596ef01f Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Tue, 7 Dec 2021 17:14:55 -0500 Subject: [PATCH 2/5] Consensus transaction recovery / deferral completely ignores the TxQ --- src/ripple/app/ledger/OpenLedger.h | 15 +------ src/ripple/app/ledger/impl/OpenLedger.cpp | 33 ++-------------- src/ripple/app/main/Application.cpp | 3 +- src/ripple/app/misc/HashRouter.cpp | 10 ----- src/ripple/app/misc/HashRouter.h | 41 +------------------ src/ripple/app/misc/impl/TxQ.cpp | 14 +------ src/ripple/ledger/ApplyView.h | 13 ++---- src/test/app/HashRouter_test.cpp | 48 +++-------------------- 8 files changed, 20 insertions(+), 157 deletions(-) diff --git a/src/ripple/app/ledger/OpenLedger.h b/src/ripple/app/ledger/OpenLedger.h index 42120cf63d1..e3cb2b10028 100644 --- a/src/ripple/app/ledger/OpenLedger.h +++ b/src/ripple/app/ledger/OpenLedger.h @@ -185,7 +185,6 @@ class OpenLedger FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - std::map& shouldRecover, beast::Journal j); enum Result { success, failure, retry }; @@ -200,7 +199,6 @@ class OpenLedger std::shared_ptr const& tx, bool retry, ApplyFlags flags, - bool shouldRecover, beast::Journal j); }; @@ -215,7 +213,6 @@ OpenLedger::apply( FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - std::map& shouldRecover, beast::Journal j) { for (auto iter = txs.begin(); iter != txs.end(); ++iter) @@ -227,8 +224,7 @@ OpenLedger::apply( auto const txId = tx->getTransactionID(); if (check.txExists(txId)) continue; - auto const result = - apply_one(app, view, tx, true, flags, shouldRecover[txId], j); + auto const result = apply_one(app, view, tx, true, flags, j); if (result == Result::retry) retries.insert(tx); } @@ -245,14 +241,7 @@ OpenLedger::apply( auto iter = retries.begin(); while (iter != retries.end()) { - switch (apply_one( - app, - view, - iter->second, - retry, - flags, - shouldRecover[iter->second->getTransactionID()], - j)) + switch (apply_one(app, view, iter->second, retry, flags, j)) { case Result::success: ++changes; diff --git a/src/ripple/app/ledger/impl/OpenLedger.cpp b/src/ripple/app/ledger/impl/OpenLedger.cpp index 113c46951a9..5bb6e53ace6 100644 --- a/src/ripple/app/ledger/impl/OpenLedger.cpp +++ b/src/ripple/app/ledger/impl/OpenLedger.cpp @@ -81,17 +81,11 @@ OpenLedger::accept( { JLOG(j_.trace()) << "accept ledger " << ledger->seq() << " " << suffix; auto next = create(rules, ledger); - std::map shouldRecover; if (retriesFirst) { - for (auto const& tx : retries) - { - auto const txID = tx.second->getTransactionID(); - shouldRecover[txID] = app.getHashRouter().shouldRecover(txID); - } // Handle disputed tx, outside lock using empty = std::vector>; - apply(app, *next, *ledger, empty{}, retries, flags, shouldRecover, j_); + apply(app, *next, *ledger, empty{}, retries, flags, j_); } // Block calls to modify, otherwise // new tx going into the open ledger @@ -100,17 +94,6 @@ OpenLedger::accept( // Apply tx from the current open view if (!current_->txs.empty()) { - for (auto const& tx : current_->txs) - { - auto const txID = tx.first->getTransactionID(); - auto iter = shouldRecover.lower_bound(txID); - if (iter != shouldRecover.end() && iter->first == txID) - // already had a chance via disputes - iter->second = false; - else - shouldRecover.emplace_hint( - iter, txID, app.getHashRouter().shouldRecover(txID)); - } apply( app, *next, @@ -124,7 +107,6 @@ OpenLedger::accept( }), retries, flags, - shouldRecover, j_); } // Call the modifier @@ -179,21 +161,12 @@ OpenLedger::apply_one( std::shared_ptr const& tx, bool retry, ApplyFlags flags, - bool shouldRecover, beast::Journal j) -> Result { if (retry) flags = flags | tapRETRY; - auto const result = [&] { - auto const queueResult = - app.getTxQ().apply(app, view, tx, flags | tapPREFER_QUEUE, j); - // If the transaction can't get into the queue for intrinsic - // reasons, and it can still be recovered, try to put it - // directly into the open ledger, else drop it. - if (queueResult.first == telCAN_NOT_QUEUE && shouldRecover) - return ripple::apply(app, view, *tx, flags, j); - return queueResult; - }(); + // If it's in anybody's proposed set, try to keep it in the ledger + auto const result = ripple::apply(app, view, *tx, flags, j); if (result.second || result.first == terQUEUED) return Result::success; if (isTefFailure(result.first) || isTemMalformed(result.first) || diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 29451bfc32d..d456209ad00 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -443,8 +443,7 @@ class ApplicationImp : public Application, public BasicApp , hashRouter_(std::make_unique( stopwatch(), - HashRouter::getDefaultHoldTime(), - HashRouter::getDefaultRecoverLimit())) + HashRouter::getDefaultHoldTime())) , mValidations( ValidationParms(), diff --git a/src/ripple/app/misc/HashRouter.cpp b/src/ripple/app/misc/HashRouter.cpp index 8a8170f486a..8085d6892ab 100644 --- a/src/ripple/app/misc/HashRouter.cpp +++ b/src/ripple/app/misc/HashRouter.cpp @@ -128,14 +128,4 @@ HashRouter::shouldRelay(uint256 const& key) return s.releasePeerSet(); } -bool -HashRouter::shouldRecover(uint256 const& key) -{ - std::lock_guard lock(mutex_); - - auto& s = emplace(key).first; - - return s.shouldRecover(recoverLimit_); -} - } // namespace ripple diff --git a/src/ripple/app/misc/HashRouter.h b/src/ripple/app/misc/HashRouter.h index a43bc1278f9..8c546b2c51d 100644 --- a/src/ripple/app/misc/HashRouter.h +++ b/src/ripple/app/misc/HashRouter.h @@ -116,20 +116,6 @@ class HashRouter return true; } - /** Determines if this item should be recovered from the open ledger. - - Counts the number of times the item has been recovered. - Every `limit` times the function is called, return false. - Else return true. - - @note The limit must be > 0 - */ - bool - shouldRecover(std::uint32_t limit) - { - return ++recoveries_ % limit != 0; - } - bool shouldProcess(Stopwatch::time_point now, std::chrono::seconds interval) { @@ -146,7 +132,6 @@ class HashRouter // than one flag needs to expire independently. std::optional relayed_; std::optional processed_; - std::uint32_t recoveries_ = 0; }; public: @@ -158,19 +143,8 @@ class HashRouter return 300s; } - static inline std::uint32_t - getDefaultRecoverLimit() - { - return 1; - } - - HashRouter( - Stopwatch& clock, - std::chrono::seconds entryHoldTimeInSeconds, - std::uint32_t recoverLimit) - : suppressionMap_(clock) - , holdTime_(entryHoldTimeInSeconds) - , recoverLimit_(recoverLimit + 1u) + HashRouter(Stopwatch& clock, std::chrono::seconds entryHoldTimeInSeconds) + : suppressionMap_(clock), holdTime_(entryHoldTimeInSeconds) { } @@ -231,15 +205,6 @@ class HashRouter std::optional> shouldRelay(uint256 const& key); - /** Determines whether the hashed item should be recovered - from the open ledger into the next open ledger or the transaction - queue. - - @return `bool` indicates whether the item should be recovered - */ - bool - shouldRecover(uint256 const& key); - private: // pair.second indicates whether the entry was created std::pair @@ -256,8 +221,6 @@ class HashRouter suppressionMap_; std::chrono::seconds const holdTime_; - - std::uint32_t const recoverLimit_; }; } // namespace ripple diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 40d6aced37e..1315bb6e2ee 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -1176,8 +1176,7 @@ TxQ::apply( for some other reason. Tx is allowed to queue in case conditions change, but don't waste the effort to clear). */ - if (!(flags & tapPREFER_QUEUE) && txSeqProx.isSeq() && txIter && - multiTxn.has_value() && + if (txSeqProx.isSeq() && txIter && multiTxn.has_value() && txIter->first->second.retriesRemaining == MaybeTx::retriesAllowed && feeLevelPaid > requiredFeeLevel && requiredFeeLevel > baseLevel) { @@ -1307,9 +1306,6 @@ TxQ::apply( // 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}); @@ -1612,13 +1608,7 @@ TxQ::getRequiredFeeLevel( FeeMetrics::Snapshot const& metricsSnapshot, std::lock_guard const& lock) const { - FeeLevel64 const feeLevel = - FeeMetrics::scaleFeeLevel(metricsSnapshot, view); - - if ((flags & tapPREFER_QUEUE) && !byFee_.empty()) - return std::max(feeLevel, byFee_.begin()->feeLevel); - - return feeLevel; + return FeeMetrics::scaleFeeLevel(metricsSnapshot, view); } std::optional> diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index b99a22ae164..5394acc78ad 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -37,11 +37,6 @@ enum ApplyFlags : std::uint32_t { // Transaction can be retried, soft failures allowed tapRETRY = 0x20, - // Transaction must pay more than both the open ledger - // fee and all transactions in the queue to get into the - // open ledger - tapPREFER_QUEUE = 0x40, - // Transaction came from a privileged source tapUNLIMITED = 0x400, }; @@ -55,10 +50,10 @@ operator|(ApplyFlags const& lhs, ApplyFlags const& rhs) } static_assert( - (tapPREFER_QUEUE | tapRETRY) == safe_cast(0x60u), + (tapFAIL_HARD | tapRETRY) == safe_cast(0x30u), "ApplyFlags operator |"); static_assert( - (tapRETRY | tapPREFER_QUEUE) == safe_cast(0x60u), + (tapRETRY | tapFAIL_HARD) == safe_cast(0x30u), "ApplyFlags operator |"); constexpr ApplyFlags @@ -69,8 +64,8 @@ operator&(ApplyFlags const& lhs, ApplyFlags const& rhs) safe_cast>(rhs)); } -static_assert((tapPREFER_QUEUE & tapRETRY) == tapNONE, "ApplyFlags operator &"); -static_assert((tapRETRY & tapPREFER_QUEUE) == tapNONE, "ApplyFlags operator &"); +static_assert((tapFAIL_HARD & tapRETRY) == tapNONE, "ApplyFlags operator &"); +static_assert((tapRETRY & tapFAIL_HARD) == tapNONE, "ApplyFlags operator &"); constexpr ApplyFlags operator~(ApplyFlags const& flags) diff --git a/src/test/app/HashRouter_test.cpp b/src/test/app/HashRouter_test.cpp index 9162fb9b1ed..96d14e824cf 100644 --- a/src/test/app/HashRouter_test.cpp +++ b/src/test/app/HashRouter_test.cpp @@ -31,7 +31,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -68,7 +68,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -146,7 +146,7 @@ class HashRouter_test : public beast::unit_test::suite // Normal HashRouter using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); uint256 const key2(2); @@ -174,7 +174,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 2s, 2); + HashRouter router(stopwatch, 2s); uint256 const key1(1); BEAST_EXPECT(router.setFlags(key1, 10)); @@ -187,7 +187,7 @@ class HashRouter_test : public beast::unit_test::suite { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 1s, 2); + HashRouter router(stopwatch, 1s); uint256 const key1(1); @@ -225,47 +225,12 @@ class HashRouter_test : public beast::unit_test::suite BEAST_EXPECT(peers && peers->size() == 0); } - void - testRecover() - { - using namespace std::chrono_literals; - TestStopwatch stopwatch; - HashRouter router(stopwatch, 1s, 5); - - uint256 const key1(1); - - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(!router.shouldRecover(key1)); - // Expire, but since the next search will - // be for this entry, it will get refreshed - // instead. - ++stopwatch; - BEAST_EXPECT(router.shouldRecover(key1)); - // Expire, but since the next search will - // be for this entry, it will get refreshed - // instead. - ++stopwatch; - // Recover again. Recovery is independent of - // time as long as the entry doesn't expire. - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(router.shouldRecover(key1)); - // Expire again - ++stopwatch; - BEAST_EXPECT(router.shouldRecover(key1)); - BEAST_EXPECT(!router.shouldRecover(key1)); - } - void testProcess() { using namespace std::chrono_literals; TestStopwatch stopwatch; - HashRouter router(stopwatch, 5s, 5); + HashRouter router(stopwatch, 5s); uint256 const key(1); HashRouter::PeerShortID peer = 1; int flags; @@ -286,7 +251,6 @@ class HashRouter_test : public beast::unit_test::suite testSuppression(); testSetFlags(); testRelay(); - testRecover(); testProcess(); } }; From fc6015ba3c0390176bca07bb49c84afd45f2878a Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 8 Dec 2021 16:59:01 -0500 Subject: [PATCH 3/5] Make transaction queue order deterministic: * Sort by fee level (which is the current behavior) then by transaction ID (hash). * Edge case when the account at the end of the queue submits a higher paying transaction to walk backwards and compare against the cheapest transaction from a different account. * Use std::if_any to simplify the JobQueue::isOverloaded loop. --- src/ripple/app/misc/TxQ.h | 25 +++-- src/ripple/app/misc/impl/TxQ.cpp | 37 +++---- src/ripple/core/impl/JobQueue.cpp | 10 +- src/test/app/TxQ_test.cpp | 163 +++++++++++++++++------------- src/test/rpc/LedgerRPC_test.cpp | 88 ++++++++++------ 5 files changed, 186 insertions(+), 137 deletions(-) diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 5f1d41c0104..e375664aaad 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -613,17 +613,30 @@ class TxQ } }; - /// Used for sorting @ref MaybeTx by `feeLevel` - class GreaterFee + /// Used for sorting @ref MaybeTx + class OrderCandidates { public: /// Default constructor - explicit GreaterFee() = default; - - /// Is the fee level of `lhs` greater than the fee level of `rhs`? + explicit OrderCandidates() = default; + + /** Sort @ref MaybeTx by `feeLevel` descending, then by + * transaction ID ascending + * + * The transaction queue is ordered such that transactions + * paying a higher fee are in front of transactions paying + * a lower fee, giving them an opportunity to be processed into + * the open ledger first. Within transactions paying the same + * fee, order by the arbitrary but consistent transaction ID. + * This allows validators to build similar queues in the same + * order, and thus have more similar initial proposals. + * + */ bool operator()(const MaybeTx& lhs, const MaybeTx& rhs) const { + if (lhs.feeLevel == rhs.feeLevel) + return lhs.txID < rhs.txID; return lhs.feeLevel > rhs.feeLevel; } }; @@ -722,7 +735,7 @@ class TxQ &MaybeTx::byFeeListHook>; using FeeMultiSet = boost::intrusive:: - multiset>; + multiset>; using AccountMap = std::map; diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 1315bb6e2ee..04d06e04fe8 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -466,30 +466,15 @@ TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter) assert(byFee_.iterator_to(accountIter->second) == candidateIter); auto const accountNextIter = std::next(accountIter); - // Check if the next transaction for this account has a greater - // SeqProxy, and a higher fee level, which means we skipped it - // earlier, and need to try it again. - // - // Edge cases: - // o 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. - // - // o If the next tx has an equal fee level, it was... - // - // * EITHER submitted later, so it's also going to be later in the - // fee queue, - // - // * OR the current was resubmitted to bump up the fee level, and - // we have skipped that next tx. - // - // In the latter case, continue through the fee queue anyway - // to head off potential ordering manipulation problems. + // Check if the next transaction for this account is earlier in the queue, + // which means we skipped it earlier, and need to try it again. + OrderCandidates o; auto const feeNextIter = std::next(candidateIter); bool const useAccountNext = accountNextIter != txQAccount.transactions.end() && accountNextIter->first > candidateIter->seqProxy && (feeNextIter == byFee_.end() || - accountNextIter->second.feeLevel > feeNextIter->feeLevel); + o(accountNextIter->second, *feeNextIter)); auto const candidateNextIter = byFee_.erase(candidateIter); txQAccount.transactions.erase(accountIter); @@ -1224,9 +1209,19 @@ TxQ::apply( if (!replacedTxIter && isFull()) { auto lastRIter = byFee_.rbegin(); - if (lastRIter->account == account) + while (lastRIter != byFee_.rend() && lastRIter->account == account) { - JLOG(j_.warn()) + ++lastRIter; + } + if (lastRIter == byFee_.rend()) + { + // The only way this condition can happen is if the entire + // queue is filled with transactions from this account. This + // is impossible with default settings - minimum queue size + // is 2000, and an account can only have 10 transactions + // queued. However, it can occur if settings are changed, + // and there is unit test coverage. + JLOG(j_.info()) << "Queue is full, and transaction " << transactionID << " would kick a transaction from the same account (" << account << ") out of the queue."; diff --git a/src/ripple/core/impl/JobQueue.cpp b/src/ripple/core/impl/JobQueue.cpp index fc50ff603e4..6588a7078c0 100644 --- a/src/ripple/core/impl/JobQueue.cpp +++ b/src/ripple/core/impl/JobQueue.cpp @@ -168,13 +168,9 @@ JobQueue::addLoadEvents(JobType t, int count, std::chrono::milliseconds elapsed) bool JobQueue::isOverloaded() { - for (auto& x : m_jobData) - { - if (x.second.load().isOver()) - return true; - } - - return false; + return std::any_of(m_jobData.begin(), m_jobData.end(), [](auto& entry) { + return entry.second.load().isOver(); + }); } Json::Value diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 3fdf00a50f0..2fcc4c87818 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -802,7 +802,7 @@ class TxQ1_test : public beast::unit_test::suite env(noop(charlie), fee(7000), queued); env(noop(daria), fee(7000), queued); env(noop(edgar), fee(7000), queued); - env(noop(felicia), fee(7000), queued); + env(noop(felicia), fee(6999), queued); checkMetrics(env, 6, 6, 4, 3, 257); env.close(); @@ -816,8 +816,8 @@ class TxQ1_test : public beast::unit_test::suite env(noop(daria), fee(8000), queued); env(noop(daria), fee(8000), seq(env.seq(daria) + 1), queued); env(noop(edgar), fee(8000), queued); - env(noop(felicia), fee(8000), queued); - env(noop(felicia), fee(8000), seq(env.seq(felicia) + 1), queued); + env(noop(felicia), fee(7999), queued); + env(noop(felicia), fee(7999), seq(env.seq(felicia) + 1), queued); checkMetrics(env, 8, 8, 5, 4, 257, 700 * 256); env.close(); @@ -1039,7 +1039,7 @@ class TxQ1_test : public beast::unit_test::suite checkMetrics(env, 0, initQueueMax, 4, 3, 256); // Alice - price starts exploding: held - env(noop(alice), queued); + env(noop(alice), fee(11), queued); checkMetrics(env, 1, initQueueMax, 4, 3, 256); auto aliceSeq = env.seq(alice); @@ -1088,7 +1088,7 @@ class TxQ1_test : public beast::unit_test::suite BEAST_EXPECT(env.seq(charlie) == charlieSeq + 1); // Alice - fill up the queue - std::int64_t aliceFee = 20; + std::int64_t aliceFee = 27; aliceSeq = env.seq(alice); auto lastLedgerSeq = env.current()->info().seq + 2; for (auto i = 0; i < 7; i++) @@ -1096,7 +1096,7 @@ class TxQ1_test : public beast::unit_test::suite env(noop(alice), seq(aliceSeq), json(jss::LastLedgerSequence, lastLedgerSeq + i), - fee(aliceFee), + fee(--aliceFee), queued); ++aliceSeq; } @@ -1104,17 +1104,18 @@ class TxQ1_test : public beast::unit_test::suite { auto& txQ = env.app().getTxQ(); auto aliceStat = txQ.getAccountTxs(alice.id(), *env.current()); - constexpr XRPAmount fee{20}; + aliceFee = 27; auto const& baseFee = env.current()->fees().base; auto seq = env.seq(alice); BEAST_EXPECT(aliceStat.size() == 7); for (auto const& tx : aliceStat) { BEAST_EXPECT(tx.seqProxy.isSeq() && tx.seqProxy.value() == seq); - BEAST_EXPECT(tx.feeLevel == toFeeLevel(fee, baseFee)); + BEAST_EXPECT( + tx.feeLevel == toFeeLevel(XRPAmount(--aliceFee), baseFee)); BEAST_EXPECT(tx.lastValid); BEAST_EXPECT( - (tx.consequences.fee() == drops(fee) && + (tx.consequences.fee() == drops(aliceFee) && tx.consequences.potentialSpend() == drops(0) && !tx.consequences.isBlocker()) || tx.seqProxy.value() == env.seq(alice) + 6); @@ -1141,13 +1142,13 @@ class TxQ1_test : public beast::unit_test::suite // 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, 513); + checkMetrics(env, 8, 8, 5, 4, 538); // 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(telCAN_NOT_QUEUE)); - checkMetrics(env, 8, 8, 5, 4, 513); + checkMetrics(env, 8, 8, 5, 4, 538); // Alice wants this tx more than the dropped tx, // so resubmits with higher fee, but the queue @@ -1156,22 +1157,22 @@ class TxQ1_test : public beast::unit_test::suite seq(aliceSeq - 1), fee(aliceFee), ter(telCAN_NOT_QUEUE_FULL)); - checkMetrics(env, 8, 8, 5, 4, 513); + checkMetrics(env, 8, 8, 5, 4, 538); // Try to replace a middle item in the queue // without enough fee. aliceSeq = env.seq(alice) + 2; - aliceFee = 25; + aliceFee = 29; env(noop(alice), seq(aliceSeq), fee(aliceFee), ter(telCAN_NOT_QUEUE_FEE)); - checkMetrics(env, 8, 8, 5, 4, 513); + checkMetrics(env, 8, 8, 5, 4, 538); // Replace a middle item from the queue successfully ++aliceFee; env(noop(alice), seq(aliceSeq), fee(aliceFee), queued); - checkMetrics(env, 8, 8, 5, 4, 513); + checkMetrics(env, 8, 8, 5, 4, 538); env.close(); // Alice's transactions processed, along with @@ -1186,7 +1187,7 @@ class TxQ1_test : public beast::unit_test::suite // more than the minimum reserve in flight before the // last queued transaction aliceFee = - env.le(alice)->getFieldAmount(sfBalance).xrp().drops() - (59); + env.le(alice)->getFieldAmount(sfBalance).xrp().drops() - (62); env(noop(alice), seq(aliceSeq), fee(aliceFee), @@ -1334,6 +1335,9 @@ class TxQ1_test : public beast::unit_test::suite auto hankSeq = env.seq(hank); // This time, use identical fees. + + // This one gets into the queue, but gets dropped when the + // higher fee one is added later. env(noop(alice), fee(15), queued); env(noop(bob), fee(15), queued); env(noop(charlie), fee(15), queued); @@ -1341,8 +1345,6 @@ class TxQ1_test : public beast::unit_test::suite env(noop(elmo), fee(15), queued); env(noop(fred), fee(15), queued); env(noop(gwen), fee(15), queued); - // This one gets into the queue, but gets dropped when the - // higher fee one is added later. env(noop(hank), fee(15), queued); // Queue is full now. Minimum fee now reflects the @@ -1362,9 +1364,9 @@ class TxQ1_test : public beast::unit_test::suite // Queue is still full. checkMetrics(env, 8, 8, 5, 4, 385); - // alice, bob, charlie, daria, and elmo's txs + // bob, charlie, daria, elmo, and fred's txs // are processed out of the queue into the ledger, - // leaving fred and gwen's txs. hank's tx is + // leaving fred and hank's txs. alice's tx is // retried from localTxs, and put back into the // queue. env.close(); @@ -1372,45 +1374,46 @@ class TxQ1_test : public beast::unit_test::suite BEAST_EXPECT(aliceSeq + 1 == env.seq(alice)); BEAST_EXPECT(bobSeq + 1 == env.seq(bob)); - BEAST_EXPECT(charlieSeq + 2 == env.seq(charlie)); + BEAST_EXPECT(charlieSeq == env.seq(charlie)); BEAST_EXPECT(dariaSeq + 1 == env.seq(daria)); - BEAST_EXPECT(elmoSeq + 1 == env.seq(elmo)); - BEAST_EXPECT(fredSeq == env.seq(fred)); - BEAST_EXPECT(gwenSeq == env.seq(gwen)); - BEAST_EXPECT(hankSeq == env.seq(hank)); + BEAST_EXPECT(elmoSeq == env.seq(elmo)); + BEAST_EXPECT(fredSeq + 1 == env.seq(fred)); + BEAST_EXPECT(gwenSeq + 1 == env.seq(gwen)); + BEAST_EXPECT(hankSeq + 1 == env.seq(hank)); aliceSeq = env.seq(alice); bobSeq = env.seq(bob); charlieSeq = env.seq(charlie); dariaSeq = env.seq(daria); elmoSeq = env.seq(elmo); + fredSeq = env.seq(fred); // Fill up the queue again - env(noop(alice), fee(15), queued); - env(noop(alice), seq(aliceSeq + 1), fee(15), queued); - env(noop(alice), seq(aliceSeq + 2), fee(15), queued); + env(noop(fred), fee(15), queued); + env(noop(fred), seq(fredSeq + 1), fee(15), queued); + env(noop(fred), seq(fredSeq + 2), fee(15), queued); env(noop(bob), fee(15), queued); - env(noop(charlie), fee(15), queued); + env(noop(charlie), seq(charlieSeq + 2), fee(15), queued); env(noop(daria), fee(15), queued); // This one gets into the queue, but gets dropped when the // higher fee one is added later. - env(noop(elmo), fee(15), queued); + env(noop(elmo), seq(elmoSeq + 1), fee(15), queued); checkMetrics(env, 10, 10, 6, 5, 385); // Add another transaction, with a higher fee, // Not high enough to get into the ledger, but high // enough to get into the queue (and kick somebody out) - env(noop(alice), fee(100), seq(aliceSeq + 3), queued); + env(noop(fred), fee(100), seq(fredSeq + 3), queued); env.close(); checkMetrics(env, 4, 12, 7, 6, 256); - BEAST_EXPECT(fredSeq + 1 == env.seq(fred)); + BEAST_EXPECT(fredSeq + 4 == env.seq(fred)); BEAST_EXPECT(gwenSeq + 1 == env.seq(gwen)); BEAST_EXPECT(hankSeq + 1 == env.seq(hank)); - BEAST_EXPECT(aliceSeq + 4 == env.seq(alice)); - BEAST_EXPECT(bobSeq == env.seq(bob)); - BEAST_EXPECT(charlieSeq == env.seq(charlie)); + BEAST_EXPECT(aliceSeq == env.seq(alice)); + BEAST_EXPECT(bobSeq + 1 == env.seq(bob)); + BEAST_EXPECT(charlieSeq + 2 == env.seq(charlie)); BEAST_EXPECT(dariaSeq == env.seq(daria)); BEAST_EXPECT(elmoSeq == env.seq(elmo)); } @@ -2767,7 +2770,7 @@ class TxQ1_test : public beast::unit_test::suite // we only see a reduction by 5. env.close(); checkMetrics(env, 9, 50, 6, 5, 256); - BEAST_EXPECT(env.seq(alice) == aliceSeq + 16); + BEAST_EXPECT(env.seq(alice) == aliceSeq + 15); // Close ledger 7. That should remove 7 more of alice's transactions. env.close(); @@ -2775,7 +2778,7 @@ class TxQ1_test : public beast::unit_test::suite BEAST_EXPECT(env.seq(alice) == aliceSeq + 19); // Close one last ledger to see all of alice's transactions moved - // into the ledger. + // into the ledger, including the tickets env.close(); checkMetrics(env, 0, 70, 2, 7, 256); BEAST_EXPECT(env.seq(alice) == aliceSeq + 21); @@ -4130,7 +4133,7 @@ class TxQ1_test : public beast::unit_test::suite {{"minimum_txn_in_ledger_standalone", "1"}, {"ledgers_in_queue", "5"}, {"maximum_txn_per_account", "10"}}, - {{"account_reserve", "200"}, {"owner_reserve", "50"}}); + {{"account_reserve", "1000"}, {"owner_reserve", "50"}}); Env env(*this, std::move(cfg)); @@ -4184,14 +4187,16 @@ class TxQ1_test : public beast::unit_test::suite auto seqDaria = env.seq(daria); auto seqEllie = env.seq(ellie); auto seqFiona = env.seq(fiona); + // Use fees to guarantee order + int txFee{90}; for (int i = 0; i < 10; ++i) { - env(noop(alice), seq(seqAlice++), ter(terQUEUED)); - env(noop(bob), seq(seqBob++), ter(terQUEUED)); - env(noop(carol), seq(seqCarol++), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), ter(terQUEUED)); - env(noop(fiona), seq(seqFiona++), ter(terQUEUED)); + env(noop(alice), seq(seqAlice++), fee(--txFee), ter(terQUEUED)); + env(noop(bob), seq(seqBob++), fee(--txFee), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--txFee), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--txFee), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--txFee), ter(terQUEUED)); + env(noop(fiona), seq(seqFiona++), fee(--txFee), ter(terQUEUED)); } std::size_t expectedInQueue = 60; checkMetrics( @@ -4283,8 +4288,8 @@ class TxQ1_test : public beast::unit_test::suite // We'll be using fees to control which entries leave the queue in // which order. There's no "lowFee" -- that's the default fee from // the unit test. - auto const medFee = drops(15); - auto const hiFee = drops(1000); + int const medFee = 100; + int const hiFee = 1000; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "5"}, @@ -4314,12 +4319,14 @@ class TxQ1_test : public beast::unit_test::suite // will expire out soon. auto seqAlice = env.seq(alice); auto const seqSaveAlice = seqAlice; + int feeDrops = 40; env(noop(alice), seq(seqAlice++), + fee(--feeDrops), json(R"({"LastLedgerSequence": 7})"), ter(terQUEUED)); - env(noop(alice), seq(seqAlice++), ter(terQUEUED)); - env(noop(alice), seq(seqAlice++), ter(terQUEUED)); + env(noop(alice), seq(seqAlice++), fee(--feeDrops), ter(terQUEUED)); + env(noop(alice), seq(seqAlice++), fee(--feeDrops), ter(terQUEUED)); BEAST_EXPECT(env.seq(alice) == seqSaveAlice); // Similarly for bob, but bob uses tickets in his transactions. @@ -4328,8 +4335,14 @@ class TxQ1_test : public beast::unit_test::suite ticket::use(bobTicketSeq + 0), json(R"({"LastLedgerSequence": 7})"), ter(terQUEUED)); - env(noop(bob), ticket::use(bobTicketSeq + 1), ter(terQUEUED)); - env(noop(bob), ticket::use(bobTicketSeq + 2), ter(terQUEUED)); + env(noop(bob), + ticket::use(bobTicketSeq + 1), + fee(--feeDrops), + ter(terQUEUED)); + env(noop(bob), + ticket::use(bobTicketSeq + 2), + fee(--feeDrops), + ter(terQUEUED)); // Fill the queue with higher fee transactions so alice's and // bob's transactions are stuck in the queue. @@ -4337,12 +4350,13 @@ class TxQ1_test : public beast::unit_test::suite auto seqDaria = env.seq(daria); auto seqEllie = env.seq(ellie); auto seqFiona = env.seq(fiona); + feeDrops = medFee; for (int i = 0; i < 7; ++i) { - env(noop(carol), seq(seqCarol++), fee(medFee), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), fee(medFee), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), fee(medFee), ter(terQUEUED)); - env(noop(fiona), seq(seqFiona++), fee(medFee), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--feeDrops), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--feeDrops), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--feeDrops), ter(terQUEUED)); + env(noop(fiona), seq(seqFiona++), fee(--feeDrops), ter(terQUEUED)); } checkMetrics(env, 34, 50, 7, 6, 256); @@ -4350,24 +4364,26 @@ class TxQ1_test : public beast::unit_test::suite checkMetrics(env, 26, 50, 8, 7, 256); // Re-fill the queue so alice and bob stay stuck. + feeDrops = medFee; for (int i = 0; i < 3; ++i) { - env(noop(carol), seq(seqCarol++), fee(medFee), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), fee(medFee), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), fee(medFee), ter(terQUEUED)); - env(noop(fiona), seq(seqFiona++), fee(medFee), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--feeDrops), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--feeDrops), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--feeDrops), ter(terQUEUED)); + env(noop(fiona), seq(seqFiona++), fee(--feeDrops), ter(terQUEUED)); } checkMetrics(env, 38, 50, 8, 7, 256); env.close(); checkMetrics(env, 29, 50, 9, 8, 256); // One more time... + feeDrops = medFee; for (int i = 0; i < 3; ++i) { - env(noop(carol), seq(seqCarol++), fee(medFee), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), fee(medFee), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), fee(medFee), ter(terQUEUED)); - env(noop(fiona), seq(seqFiona++), fee(medFee), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--feeDrops), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--feeDrops), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--feeDrops), ter(terQUEUED)); + env(noop(fiona), seq(seqFiona++), fee(--feeDrops), ter(terQUEUED)); } checkMetrics(env, 41, 50, 9, 8, 256); env.close(); @@ -4382,16 +4398,17 @@ class TxQ1_test : public beast::unit_test::suite env(noop(alice), seq(seqAlice), fee(hiFee), ter(telCAN_NOT_QUEUE)); // Once again, fill the queue almost to the brim. + feeDrops = medFee; for (int i = 0; i < 4; ++i) { - env(noop(carol), seq(seqCarol++), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), ter(terQUEUED)); - env(noop(fiona), seq(seqFiona++), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--feeDrops), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--feeDrops), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--feeDrops), ter(terQUEUED)); + env(noop(fiona), seq(seqFiona++), fee(--feeDrops), ter(terQUEUED)); } - env(noop(carol), seq(seqCarol++), ter(terQUEUED)); - env(noop(daria), seq(seqDaria++), ter(terQUEUED)); - env(noop(ellie), seq(seqEllie++), ter(terQUEUED)); + env(noop(carol), seq(seqCarol++), fee(--feeDrops), ter(terQUEUED)); + env(noop(daria), seq(seqDaria++), fee(--feeDrops), ter(terQUEUED)); + env(noop(ellie), seq(seqEllie++), fee(--feeDrops), ter(terQUEUED)); checkMetrics(env, 48, 50, 10, 9, 256); // Now induce a fee jump which should cause all the transactions @@ -4401,7 +4418,7 @@ class TxQ1_test : public beast::unit_test::suite // asynchronously lowered by LoadManager. Here we're just // pushing the local fee up really high and then hoping that we // outrace LoadManager undoing our work. - for (int i = 0; i < 10; ++i) + for (int i = 0; i < 30; ++i) env.app().getFeeTrack().raiseLocalFee(); // Now close the ledger, which will attempt to process alice's @@ -4442,7 +4459,7 @@ class TxQ1_test : public beast::unit_test::suite // Verify that there's a gap at the front of alice's queue by // queuing another low fee transaction into that spot. - env(noop(alice), seq(seqAlice++), ter(terQUEUED)); + env(noop(alice), seq(seqAlice++), fee(11), ter(terQUEUED)); // Verify that the first entry in alice's queue is still there // by trying to replace it and having that fail. @@ -4468,11 +4485,11 @@ class TxQ1_test : public beast::unit_test::suite // Verify that bob's first transaction was removed from the queue // by queueing another low fee transaction into that spot. - env(noop(bob), ticket::use(bobTicketSeq + 0), ter(terQUEUED)); + env(noop(bob), ticket::use(bobTicketSeq + 0), fee(12), ter(terQUEUED)); // Verify that bob's second transaction was removed from the queue // by queueing another low fee transaction into that spot. - env(noop(bob), ticket::use(bobTicketSeq + 1), ter(terQUEUED)); + env(noop(bob), ticket::use(bobTicketSeq + 1), fee(11), ter(terQUEUED)); // Verify that the last entry in bob's queue is still there // by trying to replace it and having that fail. diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 13a2f9824f2..df8bebfacee 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1538,21 +1538,37 @@ class LedgerRPC_test : public beast::unit_test::suite env.close(); jrr = env.rpc("json", "ledger", to_string(jv))[jss::result]; - std::string txid1; - std::string txid2; - if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) - { - auto const& txj = jrr[jss::queue_data][0u]; - BEAST_EXPECT(txj[jss::account] == alice.human()); - BEAST_EXPECT(txj[jss::fee_level] == "256"); - BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); - BEAST_EXPECT(txj["retries_remaining"] == 10); - BEAST_EXPECT(txj.isMember(jss::tx)); - auto const& tx = txj[jss::tx]; - BEAST_EXPECT(tx[jss::Account] == alice.human()); - BEAST_EXPECT(tx[jss::TransactionType] == jss::OfferCreate); - txid1 = tx[jss::hash].asString(); - } + const std::string txid1 = [&]() { + if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) + { + const std::string txid0 = [&]() { + auto const& txj = jrr[jss::queue_data][0u]; + BEAST_EXPECT(txj[jss::account] == alice.human()); + BEAST_EXPECT(txj[jss::fee_level] == "256"); + BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); + BEAST_EXPECT(txj["retries_remaining"] == 10); + BEAST_EXPECT(txj.isMember(jss::tx)); + auto const& tx = txj[jss::tx]; + BEAST_EXPECT(tx[jss::Account] == alice.human()); + BEAST_EXPECT(tx[jss::TransactionType] == jss::AccountSet); + return tx[jss::hash].asString(); + }(); + + auto const& txj = jrr[jss::queue_data][1u]; + BEAST_EXPECT(txj[jss::account] == alice.human()); + BEAST_EXPECT(txj[jss::fee_level] == "256"); + BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); + BEAST_EXPECT(txj["retries_remaining"] == 10); + BEAST_EXPECT(txj.isMember(jss::tx)); + auto const& tx = txj[jss::tx]; + BEAST_EXPECT(tx[jss::Account] == alice.human()); + BEAST_EXPECT(tx[jss::TransactionType] == jss::OfferCreate); + const auto txid1 = tx[jss::hash].asString(); + BEAST_EXPECT(txid0 < txid1); + return txid1; + } + return std::string{}; + }(); env.close(); @@ -1561,7 +1577,15 @@ class LedgerRPC_test : public beast::unit_test::suite jrr = env.rpc("json", "ledger", to_string(jv))[jss::result]; if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) { - auto const& txj = jrr[jss::queue_data][0u]; + auto const txid0 = [&]() { + auto const& txj = jrr[jss::queue_data][0u]; + BEAST_EXPECT(txj[jss::account] == alice.human()); + BEAST_EXPECT(txj[jss::fee_level] == "256"); + BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); + BEAST_EXPECT(txj.isMember(jss::tx)); + return txj[jss::tx].asString(); + }(); + auto const& txj = jrr[jss::queue_data][1u]; BEAST_EXPECT(txj[jss::account] == alice.human()); BEAST_EXPECT(txj[jss::fee_level] == "256"); BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); @@ -1569,6 +1593,7 @@ class LedgerRPC_test : public beast::unit_test::suite BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ"); BEAST_EXPECT(txj.isMember(jss::tx)); BEAST_EXPECT(txj[jss::tx] == txid1); + BEAST_EXPECT(txid0 < txid1); } env.close(); @@ -1579,7 +1604,7 @@ class LedgerRPC_test : public beast::unit_test::suite jrr = env.rpc("json", "ledger", to_string(jv))[jss::result]; if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) { - auto const& txj = jrr[jss::queue_data][0u]; + auto const& txj = jrr[jss::queue_data][1u]; BEAST_EXPECT(txj[jss::account] == alice.human()); BEAST_EXPECT(txj[jss::fee_level] == "256"); BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); @@ -1588,7 +1613,7 @@ class LedgerRPC_test : public beast::unit_test::suite BEAST_EXPECT(txj.isMember(jss::tx)); BEAST_EXPECT(txj[jss::tx].isMember(jss::tx_blob)); - auto const& txj2 = jrr[jss::queue_data][1u]; + auto const& txj2 = jrr[jss::queue_data][0u]; BEAST_EXPECT(txj2[jss::account] == alice.human()); BEAST_EXPECT(txj2[jss::fee_level] == "256"); BEAST_EXPECT(txj2["preflight_result"] == "tesSUCCESS"); @@ -1607,18 +1632,21 @@ class LedgerRPC_test : public beast::unit_test::suite jv[jss::binary] = false; jrr = env.rpc("json", "ledger", to_string(jv))[jss::result]; - if (BEAST_EXPECT(jrr[jss::queue_data].size() == 1)) - { - auto const& txj = jrr[jss::queue_data][0u]; - BEAST_EXPECT(txj[jss::account] == alice.human()); - BEAST_EXPECT(txj[jss::fee_level] == "256"); - BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); - BEAST_EXPECT(txj["retries_remaining"] == 1); - BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ"); - BEAST_EXPECT(txj.isMember(jss::tx)); - BEAST_EXPECT(txj[jss::tx] != txid1); - txid2 = txj[jss::tx].asString(); - } + const std::string txid2 = [&]() { + if (BEAST_EXPECT(jrr[jss::queue_data].size() == 1)) + { + auto const& txj = jrr[jss::queue_data][0u]; + BEAST_EXPECT(txj[jss::account] == alice.human()); + BEAST_EXPECT(txj[jss::fee_level] == "256"); + BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); + BEAST_EXPECT(txj["retries_remaining"] == 1); + BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ"); + BEAST_EXPECT(txj.isMember(jss::tx)); + BEAST_EXPECT(txj[jss::tx] != txid1); + return txj[jss::tx].asString(); + } + return std::string{}; + }(); jv[jss::full] = true; From 010f0cf55e9bffe384e3ec730179cb1c6dc07a42 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 8 Dec 2021 21:57:19 -0500 Subject: [PATCH 4/5] Reduce TxQ logging severity in several places --- src/ripple/app/misc/impl/TxQ.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 04d06e04fe8..196060a5bc6 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -98,12 +98,11 @@ TxQ::FeeMetrics::update( std::sort(feeLevels.begin(), feeLevels.end()); assert(size == feeLevels.size()); - JLOG(j_.debug()) << "Ledger " << view.info().seq << " has " << size - << " transactions. " - << "Ledgers are processing " - << (timeLeap ? "slowly" : "as expected") - << ". Expected transactions is currently " << txnsExpected_ - << " and multiplier is " << escalationMultiplier_; + JLOG((timeLeap ? j_.warn() : j_.debug())) + << "Ledger " << view.info().seq << " has " << size << " transactions. " + << "Ledgers are processing " << (timeLeap ? "slowly" : "as expected") + << ". Expected transactions is currently " << txnsExpected_ + << " and multiplier is " << escalationMultiplier_; if (timeLeap) { @@ -1262,7 +1261,7 @@ TxQ::apply( // valuable, so kick out the cheapest transaction. auto dropRIter = endAccount.transactions.rbegin(); assert(dropRIter->second.account == lastRIter->account); - JLOG(j_.warn()) + JLOG(j_.info()) << "Removing last item of account " << lastRIter->account << " from queue with average fee of " << endEffectiveFeeLevel << " in favor of " << transactionID << " with fee of " @@ -1271,7 +1270,7 @@ TxQ::apply( } else { - JLOG(j_.warn()) + JLOG(j_.info()) << "Queue is full, and transaction " << transactionID << " fee is lower than end item's account average fee"; return {telCAN_NOT_QUEUE_FULL, false}; @@ -1489,7 +1488,7 @@ TxQ::accept(Application& app, OpenView& view) { // Since the failed transaction has a ticket, order // doesn't matter. Drop this one. - JLOG(j_.warn()) + JLOG(j_.info()) << "Queue is nearly full, and transaction " << candidateIter->txID << " failed with " << transToken(txnResult) @@ -1508,7 +1507,7 @@ TxQ::accept(Application& app, OpenView& view) dropRIter->second.account == candidateIter->account); - JLOG(j_.warn()) + JLOG(j_.info()) << "Queue is nearly full, and transaction " << candidateIter->txID << " failed with " << transToken(txnResult) From aa20afa6d41939fd5cf950b9c5a1fbfe2fa3db37 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Thu, 9 Dec 2021 19:59:31 -0500 Subject: [PATCH 5/5] Increase TxQ default minimum and target sizes --- src/ripple/app/misc/TxQ.h | 4 ++-- src/test/app/TxQ_test.cpp | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index e375664aaad..0c642c1d955 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -96,13 +96,13 @@ class TxQ FeeLevel64 minimumEscalationMultiplier = baseLevel * 500; /// Minimum number of transactions to allow into the ledger /// before escalation, regardless of the prior ledger's size. - std::uint32_t minimumTxnInLedger = 5; + std::uint32_t minimumTxnInLedger = 32; /// Like @ref minimumTxnInLedger for standalone mode. /// Primarily so that tests don't need to worry about queuing. std::uint32_t minimumTxnInLedgerSA = 1000; /// Number of transactions per ledger that fee escalation "works /// towards". - std::uint32_t targetTxnInLedger = 50; + std::uint32_t targetTxnInLedger = 256; /** Optional maximum allowed value of transactions per ledger before fee escalation kicks in. By default, the maximum is an emergent property of network, validator, and consensus performance. This diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 2fcc4c87818..2ade9e8e307 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1469,6 +1469,7 @@ class TxQ1_test : public beast::unit_test::suite *this, makeConfig( {{"minimum_txn_in_ledger_standalone", "2"}, + {"minimum_txn_in_ledger", "5"}, {"target_txn_in_ledger", "4"}, {"maximum_txn_in_ledger", "5"}}));