From ebc2a9a6250171146820efe555553b68d2d76f11 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Mon, 15 Sep 2025 14:51:19 +0100 Subject: [PATCH] fix: Skip processing transaction batch if the batch is empty (#5670) Avoids an assertion failure in NetworkOPsImp::apply in the unlikely event that all incoming transactions are invalid. --- src/test/app/NetworkOPs_test.cpp | 80 ++++++++++++++++++++++++++++ src/xrpld/app/misc/NetworkOPs.cpp | 5 ++ src/xrpld/overlay/detail/PeerImp.cpp | 13 ++--- 3 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 src/test/app/NetworkOPs_test.cpp diff --git a/src/test/app/NetworkOPs_test.cpp b/src/test/app/NetworkOPs_test.cpp new file mode 100644 index 0000000000..edea55105b --- /dev/null +++ b/src/test/app/NetworkOPs_test.cpp @@ -0,0 +1,80 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2020 Dev Null Productions + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include + +#include + +namespace ripple { +namespace test { + +class NetworkOPs_test : public beast::unit_test::suite +{ +public: + void + run() override + { + testAllBadHeldTransactions(); + } + + void + testAllBadHeldTransactions() + { + // All trasactions are already marked as SF_BAD, and we should be able + // to handle the case properly without an assertion failure + testcase("No valid transactions in batch"); + + std::string logs; + + { + using namespace jtx; + auto const alice = Account{"alice"}; + Env env{ + *this, + envconfig(), + std::make_unique(&logs), + beast::severities::kAll}; + env.memoize(env.master); + env.memoize(alice); + + auto const jtx = env.jt(ticket::create(alice, 1), seq(1), fee(10)); + + auto transacionId = jtx.stx->getTransactionID(); + env.app().getHashRouter().setFlags( + transacionId, HashRouterFlags::HELD); + + env(jtx, json(jss::Sequence, 1), ter(terNO_ACCOUNT)); + + env.app().getHashRouter().setFlags( + transacionId, HashRouterFlags::BAD); + + env.close(); + } + + BEAST_EXPECT( + logs.find("No transaction to process!") != std::string::npos); + } +}; + +BEAST_DEFINE_TESTSUITE(NetworkOPs, app, ripple); + +} // namespace test +} // namespace ripple diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 3220ce99fc..347bbb5307 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1448,6 +1448,11 @@ NetworkOPsImp::processTransactionSet(CanonicalTXSet const& set) for (auto& t : transactions) mTransactions.push_back(std::move(t)); } + if (mTransactions.empty()) + { + JLOG(m_journal.debug()) << "No transaction to process!"; + return; + } doTransactionSyncBatch(lock, [&](std::unique_lock const&) { XRPL_ASSERT( diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 23b4760488..956f990749 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1286,8 +1286,7 @@ PeerImp::handleTransaction( // Charge strongly for attempting to relay a txn with tfInnerBatchTxn // LCOV_EXCL_START - if (stx->isFlag(tfInnerBatchTxn) && - getCurrentTransactionRules()->enabled(featureBatch)) + if (stx->isFlag(tfInnerBatchTxn)) { JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing " "tfInnerBatchTxn (handleTransaction)."; @@ -2851,8 +2850,7 @@ PeerImp::checkTransaction( { // charge strongly for relaying batch txns // LCOV_EXCL_START - if (stx->isFlag(tfInnerBatchTxn) && - getCurrentTransactionRules()->enabled(featureBatch)) + if (stx->isFlag(tfInnerBatchTxn)) { JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing " "tfInnerBatchTxn (checkSignature)."; @@ -2866,6 +2864,9 @@ PeerImp::checkTransaction( (stx->getFieldU32(sfLastLedgerSequence) < app_.getLedgerMaster().getValidLedgerIndex())) { + JLOG(p_journal_.info()) + << "Marking transaction " << stx->getTransactionID() + << "as BAD because it's expired"; app_.getHashRouter().setFlags( stx->getTransactionID(), HashRouterFlags::BAD); charge(Resource::feeUselessData, "expired tx"); @@ -2922,7 +2923,7 @@ PeerImp::checkTransaction( { if (!validReason.empty()) { - JLOG(p_journal_.trace()) + JLOG(p_journal_.debug()) << "Exception checking transaction: " << validReason; } @@ -2949,7 +2950,7 @@ PeerImp::checkTransaction( { if (!reason.empty()) { - JLOG(p_journal_.trace()) + JLOG(p_journal_.debug()) << "Exception checking transaction: " << reason; } app_.getHashRouter().setFlags(