fix: Cap TMTransactions list size and charge fee for undeserializable transactions

src/test/overlay/PeerTest.cpp

@@ -29,6 +29,7 @@
 #include <xrpl.pb.h>
 
 #include <memory>
+#include <optional>
 #include <utility>
 #include <vector>
 
@@ -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<bool> enabled)
+{
+    compressionEnabled_ = enabled;
+}
+
+bool
+PeerTest::txReduceRelayEnabled() const
+{
+    if (reduceRelayEnabled_.has_value())
+    {
+        return *reduceRelayEnabled_;
+    }
+    return PeerImp::txReduceRelayEnabled();
+}
+
+void
+PeerTest::txReduceRelayEnabled(std::optional<bool> enabled)
+{
+    reduceRelayEnabled_ = enabled;
+}
+
 std::shared_ptr<PeerTest>
 makePeerTest(jtx::Env& env, PeerTest::SharedContext const& context, ProtocolVersion protocolVersion)
 {

src/test/overlay/PeerTest.h

@@ -26,6 +26,7 @@
 #include <xrpl.pb.h>
 
 #include <memory>
+#include <optional>
 #include <vector>
 
 namespace xrpl::test {
@@ -35,9 +36,10 @@ namespace xrpl::test {
  */
 class PeerTest : public PeerImp
 {
-private:
     inline static Peer::id_t id{};
     std::shared_ptr<Message> lastSentMessage_;
+    std::optional<bool> compressionEnabled_;
+    std::optional<bool> reduceRelayEnabled_;
 
 public:
     using MiddleType = boost::beast::tcp_stream;
@@ -83,6 +85,18 @@ public:
 
     static void
     resetId();
+
+    bool
+    compressionEnabled() const override;
+
+    void
+    compressionEnabled(std::optional<bool> enabled);
+
+    bool
+    txReduceRelayEnabled() const override;
+
+    void
+    txReduceRelayEnabled(std::optional<bool> enabled);
 };
 
 std::shared_ptr<PeerTest>

src/test/overlay/TMGetLedger_test.cpp

@@ -44,13 +44,11 @@ class TMGetLedger_test : public beast::unit_test::Suite
         auto request = std::make_shared<protocol::TMGetLedger>();
         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<std::size_t>(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);
     }
 };
 

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<protocol::TMGetObjectByHash>();
         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<int>(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);
     }
 };
 

src/test/overlay/TMTransaction_test.cpp

@@ -0,0 +1,60 @@
+#include <test/jtx/Env.h>
+#include <test/jtx/envconfig.h>
+#include <test/overlay/PeerTest.h>
+
+#include <xrpld/overlay/detail/OverlayImpl.h>
+#include <xrpld/overlay/detail/PeerImp.h>
+#include <xrpld/overlay/detail/ProtocolVersion.h>
+
+#include <xrpl/basics/make_SSLContext.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/resource/Fees.h>
+
+#include <boost/asio/ip/address.hpp>
+#include <boost/asio/ip/tcp.hpp>
+#include <boost/asio/ssl/context.hpp>
+#include <boost/beast/core/tcp_stream.hpp>
+#include <boost/beast/ssl/ssl_stream.hpp>
+
+#include <xrpl.pb.h>
+
+#include <memory>
+
+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<protocol::TMTransaction>();
+        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

src/test/overlay/TMTransactions_test.cpp

@@ -0,0 +1,88 @@
+#include <test/jtx/CheckMessageLogs.h>
+#include <test/jtx/Env.h>
+#include <test/jtx/envconfig.h>
+#include <test/overlay/PeerTest.h>
+
+#include <xrpld/overlay/ReduceRelayCommon.h>
+#include <xrpld/overlay/detail/OverlayImpl.h>
+#include <xrpld/overlay/detail/PeerImp.h>
+#include <xrpld/overlay/detail/ProtocolVersion.h>
+#include <xrpld/overlay/detail/Tuning.h>
+
+#include <xrpl/basics/make_SSLContext.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/resource/Fees.h>
+
+#include <boost/asio/ip/address.hpp>
+#include <boost/asio/ip/tcp.hpp>
+#include <boost/asio/ssl/context.hpp>
+#include <boost/beast/core/tcp_stream.hpp>
+#include <boost/beast/ssl/ssl_stream.hpp>
+
+#include <xrpl.pb.h>
+
+#include <cstddef>
+#include <memory>
+
+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<protocol::TMTransactions>
+    createRequest(std::size_t const numTransactions)
+    {
+        auto request = std::make_shared<protocol::TMTransactions>();
+        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<CheckMessageLogs>(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

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<protocol::TMTransactions> 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());