diff --git a/src/test/overlay/PeerTest.cpp b/src/test/overlay/PeerTest.cpp index 6ca6db384b..341febb25b 100644 --- a/src/test/overlay/PeerTest.cpp +++ b/src/test/overlay/PeerTest.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -99,6 +100,38 @@ PeerTest::resetId() id = 0; } +bool +PeerTest::compressionEnabled() const +{ + if (compressionEnabled_.has_value()) + { + return *compressionEnabled_; + } + return PeerImp::compressionEnabled(); +} + +void +PeerTest::compressionEnabled(std::optional enabled) +{ + compressionEnabled_ = enabled; +} + +bool +PeerTest::txReduceRelayEnabled() const +{ + if (reduceRelayEnabled_.has_value()) + { + return *reduceRelayEnabled_; + } + return PeerImp::txReduceRelayEnabled(); +} + +void +PeerTest::txReduceRelayEnabled(std::optional enabled) +{ + reduceRelayEnabled_ = enabled; +} + std::shared_ptr makePeerTest(jtx::Env& env, PeerTest::SharedContext const& context, ProtocolVersion protocolVersion) { diff --git a/src/test/overlay/PeerTest.h b/src/test/overlay/PeerTest.h index a081399bac..f7b2815da3 100644 --- a/src/test/overlay/PeerTest.h +++ b/src/test/overlay/PeerTest.h @@ -26,6 +26,7 @@ #include #include +#include #include namespace xrpl::test { @@ -35,9 +36,10 @@ namespace xrpl::test { */ class PeerTest : public PeerImp { -private: inline static Peer::id_t id{}; std::shared_ptr lastSentMessage_; + std::optional compressionEnabled_; + std::optional reduceRelayEnabled_; public: using MiddleType = boost::beast::tcp_stream; @@ -83,6 +85,18 @@ public: static void resetId(); + + bool + compressionEnabled() const override; + + void + compressionEnabled(std::optional enabled); + + bool + txReduceRelayEnabled() const override; + + void + txReduceRelayEnabled(std::optional enabled); }; std::shared_ptr diff --git a/src/test/overlay/TMGetLedger_test.cpp b/src/test/overlay/TMGetLedger_test.cpp index 2f072e0efe..9088e6fa65 100644 --- a/src/test/overlay/TMGetLedger_test.cpp +++ b/src/test/overlay/TMGetLedger_test.cpp @@ -44,13 +44,11 @@ class TMGetLedger_test : public beast::unit_test::Suite auto request = std::make_shared(); request->set_itype(protocol::liTX_NODE); - // A uint256-sized ledger hash so the request passes the earlier - // structural validation and reaches the node-ID checks. + // A uint256-sized ledger hash, as a well-formed request carries. uint256 const ledgerHash{1}; request->set_ledgerhash(ledgerHash.data(), ledgerHash.size()); - // Valid, deserializable SHAMap node IDs (the root node ID, repeated). - // The count is what the hard bound cares about. + // Valid, deserializable SHAMap node IDs. auto const rootNodeId = SHAMapNodeID{}.getRawString(); for (std::size_t i = 0; i < numNodeIds; ++i) { @@ -61,9 +59,9 @@ class TMGetLedger_test : public beast::unit_test::Suite } void - testNodeIdHardLimit(std::size_t const numNodeIds, bool const expectRejected) + testNodeIdCountAccepted(std::size_t const numNodeIds, bool const expectRejected) { - testcase("Node ID Hard Limit"); + testcase("Node ID Count Accepted"); Env env{*this}; PeerTest::resetId(); @@ -71,17 +69,18 @@ class TMGetLedger_test : public beast::unit_test::Suite auto peer = makePeerTest(env, context_, protocolVersion_); peer->onMessage(createRequest(numNodeIds)); - // An over-limit request is charged kFeeInvalidData; an in-limit request should not be. - // The JobQueue handler may run concurrently and update the fee for in-limit requests. + // A request outside the accepted node-ID count is charged kFeeInvalidData; one inside + // it is not. The JobQueue handler may run concurrently and update the fee in the + // accepted case. BEAST_EXPECT( expectRejected ? (peer->getCurrentFeeCharge() == resource::kFeeInvalidData) : !(peer->getCurrentFeeCharge() == resource::kFeeInvalidData)); } void - testProcessLedgerRequestReplyCapped(std::size_t const numNodeIds) + testProcessLedgerRequestNodeCount(std::size_t const numNodeIds) { - testcase("Process Ledger Request Reply Capped"); + testcase("Process Ledger Request Node Count"); Env env{*this}; env.close(); @@ -89,8 +88,7 @@ class TMGetLedger_test : public beast::unit_test::Suite auto peer = makePeerTest(env, context_, protocolVersion_); - // Request the account-state root node of the closed ledger, asking - // for far more node IDs than the hard bound allows. + // Ask for the account-state root node of the closed ledger. auto request = createRequest(numNodeIds); request->clear_ledgerhash(); request->set_itype(protocol::liAS_NODE); @@ -121,12 +119,12 @@ class TMGetLedger_test : public beast::unit_test::Suite run() override { auto const limit = static_cast(tuning::kHardMaxReplyNodes); - testNodeIdHardLimit(limit + 1, true); - testNodeIdHardLimit(limit, false); - testNodeIdHardLimit(limit - 1, false); - testProcessLedgerRequestReplyCapped(limit + 1); - testProcessLedgerRequestReplyCapped(limit); - testProcessLedgerRequestReplyCapped(limit - 1); + testNodeIdCountAccepted(limit + 1, true); + testNodeIdCountAccepted(limit, false); + testNodeIdCountAccepted(limit - 1, false); + testProcessLedgerRequestNodeCount(limit + 1); + testProcessLedgerRequestNodeCount(limit); + testProcessLedgerRequestNodeCount(limit - 1); } }; diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp index 7c614d447c..84495fec37 100644 --- a/src/test/overlay/TMGetObjectByHash_test.cpp +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -33,11 +33,11 @@ namespace xrpl::test { using namespace jtx; /** - * Test for TMGetObjectByHash reply size limiting. + * Coverage for the TMGetObjectByHash object-count bound. * - * This verifies the fix that limits TMGetObjectByHash replies to - * tuning::hardMaxReplyNodes to prevent excessive memory usage and - * potential DoS attacks from peers requesting large numbers of objects. + * A generic query names some number of objects; the number of entries the + * reply carries is bounded by tuning::kHardMaxReplyNodes. These cases pin that + * bound at and either side of its boundary. */ class TMGetObjectByHash_test : public beast::unit_test::Suite { @@ -63,7 +63,7 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite NodeObjectType::Ledger, std::move(data), hash, nodeStore.earliestLedgerSeq()); } - // Create a request with more objects than hardMaxReplyNodes + // Name every stored object in a single generic query. auto request = std::make_shared(); request->set_type(protocol::TMGetObjectByHash_ObjectType_otLEDGER); request->set_query(true); @@ -78,17 +78,16 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite } /** - * Test that reply is limited to hardMaxReplyNodes when more objects - * are requested than the limit allows. + * Check the object count a generic-query reply carries. * * `onMessage(TMGetObjectByHash)` dispatches the generic-query path * to the JobQueue, so tests invoke the synchronous processor * directly via `runProcessGetObjectByHash`. */ void - testReplyLimit(size_t const numObjects, int const expectedReplySize) + testReplyObjectCount(size_t const numObjects, int const expectedReplySize) { - testcase("Reply Limit"); + testcase("Reply Object Count"); Env env(*this); PeerTest::resetId(); @@ -110,7 +109,7 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite protocol::TMGetObjectByHash reply; BEAST_EXPECT(reply.ParseFromArray(buffer.data() + 6, buffer.size() - 6) == true); - // Verify the reply is limited to expectedReplySize + // The reply carries the expected number of objects. BEAST_EXPECT(reply.objects_size() == expectedReplySize); } @@ -118,9 +117,9 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite run() override { int const limit = static_cast(tuning::kHardMaxReplyNodes); - testReplyLimit(limit + 1, limit); - testReplyLimit(limit, limit); - testReplyLimit(limit - 1, limit - 1); + testReplyObjectCount(limit + 1, limit); + testReplyObjectCount(limit, limit); + testReplyObjectCount(limit - 1, limit - 1); } }; diff --git a/src/test/overlay/TMTransaction_test.cpp b/src/test/overlay/TMTransaction_test.cpp new file mode 100644 index 0000000000..b208b3d81b --- /dev/null +++ b/src/test/overlay/TMTransaction_test.cpp @@ -0,0 +1,60 @@ +#include +#include +#include + +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include + +namespace xrpl::test { + +using namespace jtx; + +class TMTransaction_test : public beast::unit_test::Suite +{ + PeerTest::SharedContext context_{makeSslContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + void + testFailureDeserializingTransactionIsCharged() + { + testcase("Undeserializable Transaction Is Charged"); + + Env env{*this, envconfig()}; + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + auto tx = std::make_shared(); + tx->set_status(protocol::tsNEW); + + // Bytes that are not a serialized transaction, so deserialization fails. + tx->set_rawtransaction("\x01\x02\x03", 3); + + peer->onMessage(tx); + BEAST_EXPECT(peer->getCurrentFeeCharge() == resource::kFeeInvalidData); + } + + void + run() override + { + testFailureDeserializingTransactionIsCharged(); + } +}; + +BEAST_DEFINE_TESTSUITE(TMTransaction, overlay, xrpl); + +} // namespace xrpl::test diff --git a/src/test/overlay/TMTransactions_test.cpp b/src/test/overlay/TMTransactions_test.cpp new file mode 100644 index 0000000000..67d36cc04b --- /dev/null +++ b/src/test/overlay/TMTransactions_test.cpp @@ -0,0 +1,88 @@ +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +#include +#include + +namespace xrpl::test { + +using namespace jtx; + +class TMTransactions_test : public beast::unit_test::Suite +{ + PeerTest::SharedContext context_{makeSslContext("")}; + ProtocolVersion protocolVersion_{1, 7}; + + static std::shared_ptr + createRequest(std::size_t const numTransactions) + { + auto request = std::make_shared(); + for (std::size_t i = 0; i < numTransactions; ++i) + { + request->mutable_transactions()->Add(protocol::TMTransaction{}); + } + return request; + } + + void + testTransactionCountAccepted(std::size_t const numTransactions, bool const expectRejected) + { + testcase("Transaction Count Accepted"); + + static constexpr auto kLimitExceededMessage = "TMTransactions: transaction list too large"; + auto foundExpectedLog = false; + Env env{ + *this, + envconfig(), + std::make_unique(kLimitExceededMessage, &foundExpectedLog)}; + PeerTest::resetId(); + + auto peer = makePeerTest(env, context_, protocolVersion_); + peer->txReduceRelayEnabled(true); + peer->onMessage(createRequest(numTransactions)); + + auto fee = peer->getCurrentFeeCharge(); + if (expectRejected) + { + BEAST_EXPECT(fee == resource::kFeeMalformedRequest); + BEAST_EXPECT(foundExpectedLog); + } + else + { + BEAST_EXPECT(!foundExpectedLog); + } + } + + void + run() override + { + auto const limit = reduce_relay::kMaxTxQueueSize; + testTransactionCountAccepted(limit + 1, true); + testTransactionCountAccepted(limit, false); + testTransactionCountAccepted(limit - 1, false); + } +}; + +BEAST_DEFINE_TESTSUITE(TMTransactions, overlay, xrpl); + +} // namespace xrpl::test diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index eddeb01897..422db53389 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1414,6 +1414,10 @@ PeerImp::handleTransaction( } catch (std::exception const& ex) { + if (fee_.fee < resource::kFeeInvalidData) + { + fee_.update(resource::kFeeInvalidData, "tx invalid"); + } JLOG(pJournal_.warn()) << "Transaction invalid: " << strHex(m->rawtransaction()) << ". Exception: " << ex.what(); } @@ -2859,6 +2863,13 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } + if (m->transactions_size() > reduce_relay::kMaxTxQueueSize) + { + JLOG(pJournal_.error()) << "TMTransactions: transaction list too large"; + fee_.update(resource::kFeeMalformedRequest, "Transaction list too large"); + return; + } + JLOG(pJournal_.trace()) << "received TMTransactions " << m->transactions_size(); overlay_.addTxMetrics(m->transactions_size());