Merge branch 'develop' into bthomee/jq

.github/scripts/strategy-matrix/generate.py

@@ -20,6 +20,8 @@ _SANITIZER_SUFFIX: dict[str, str] = {
 def get_cmake_args(build_type: str, extra_args: str) -> str:
     """Get the full list of CMake arguments for a config."""
     args = _BASE_CMAKE_ARGS.copy()
+    if build_type == "Release":
+        args.append("-Dassert=ON")
     if extra_args:
         args.extend(extra_args.split())
     return " ".join(args)

include/xrpl/basics/TaggedCache.ipp

@@ -2,7 +2,6 @@
 
 #include <xrpl/basics/IntrusivePointer.ipp>
 #include <xrpl/basics/TaggedCache.h>
-#include <xrpl/basics/scope.h>
 
 namespace xrpl {
 
@@ -537,15 +536,8 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
     std::vector<key_type> v;
 
     {
-        std::unique_lock lock(mutex_);
-        for (auto size = cache_.size(); v.capacity() < size; size = cache_.size())
-        {
-            ScopeUnlock const unlock(lock);
-            v.reserve(size);
-        }
-        XRPL_ASSERT(lock.owns_lock(), "xrpl::TaggedCache::getKeys(): owns lock");
-        XRPL_ASSERT(
-            v.capacity() >= cache_.size(), "xrpl::TaggedCache::getKeys(): sufficient capacity");
+        std::scoped_lock const lock(mutex_);
+        v.reserve(cache_.size());
         for (auto const& _ : cache_)
             v.push_back(_.first);
     }

include/xrpl/basics/sanitizers.h

@@ -4,7 +4,7 @@
 /*
  ASAN flags some false positives with sudden jumps in control flow, like
  exceptions, or when encountering coroutine stack switches. This macro can be used to disable ASAN
- instrumentation for specific functions.
+ intrumentation for specific functions.
 */
 #if defined(__GNUC__) || defined(__clang__)
 #define XRPL_NO_SANITIZE_ADDRESS __attribute__((no_sanitize("address", "hwaddress")))

include/xrpl/ledger/helpers/CredentialHelpers.h

@@ -36,13 +36,13 @@ checkFields(STTx const& tx, beast::Journal j);
 TER
 valid(STTx const& tx, ReadView const& view, AccountID const& src, beast::Journal j);
 
-// Check if subject has any credential matching the given domain. If you call it
+// Check if subject has any credential maching the given domain. If you call it
 // in preclaim and it returns tecEXPIRED, you should call verifyValidDomain in
 // doApply. This will ensure that expired credentials are deleted.
 TER
 validDomain(ReadView const& view, uint256 domainID, AccountID const& subject);
 
-// This function is only called when we are about to return tecNO_PERMISSION
+// This function is only called when we about to return tecNO_PERMISSION
 // because all the checks for the DepositPreauth authorization failed.
 TER
 authorizedDepositPreauth(ReadView const& view, STVector256 const& ctx, AccountID const& dst);
@@ -58,7 +58,7 @@ checkArray(STArray const& credentials, unsigned maxSize, beast::Journal j);
 
 }  // namespace credentials
 
-// Check expired credentials and for credentials matching DomainID of the ledger
+// Check expired credentials and for credentials maching DomainID of the ledger
 // object
 TER
 verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, beast::Journal j);

nix/packages.nix

@@ -22,7 +22,6 @@ in
     git-lfs
     gnumake
     gnupg # needed for signing commits & codecov/codecov-action
-    graphviz
     llvmPackages_22.clang-tools
     less # needed for git diff
     mold

src/xrpld/overlay/detail/PeerImp.cpp

@@ -61,6 +61,7 @@
 #include <xrpl/resource/Gossip.h>
 #include <xrpl/server/LoadFeeTrack.h>
 #include <xrpl/server/NetworkOPs.h>
+#include <xrpl/shamap/SHAMap.h>
 #include <xrpl/shamap/SHAMapNodeID.h>
 #include <xrpl/tx/apply.h>
 
@@ -1507,23 +1508,12 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Verify ledger node IDs
-    if (itype != protocol::liBASE)
+    // Verify ledger node counts. Full parsing of the node IDs is deferred to the job, so the I/O
+    // thread is not burdened with SHAMapNodeID deserialization for every TMGetLedger message.
+    if (itype != protocol::liBASE && m->nodeids_size() <= 0)
     {
-        if (m->nodeids_size() <= 0)
-        {
-            badData("Invalid ledger node IDs");
-            return;
-        }
-
-        for (auto const& nodeId : m->nodeids())
-        {
-            if (deserializeSHAMapNodeID(nodeId) == std::nullopt)
-            {
-                badData("Invalid SHAMap node ID");
-                return;
-            }
-        }
+        badData("Invalid ledger node IDs");
+        return;
     }
 
     // Verify query type
@@ -1543,11 +1533,40 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetLedger> const& m)
         }
     }
 
-    // Queue a job to process the request
+    // Queue a job to process the request.
     std::weak_ptr<PeerImp> const weak = shared_from_this();
-    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m]() {
-        if (auto peer = weak.lock())
-            peer->processLedgerRequest(m);
+    app_.getJobQueue().addJob(JtLedgerReq, "RcvGetLedger", [weak, m, itype]() {
+        auto peer = weak.lock();
+        if (!peer)
+            return;
+
+        std::vector<SHAMapNodeID> nodeIDs;
+        if (itype != protocol::liBASE)
+        {
+            nodeIDs.reserve(std::min(m->nodeids_size(), Tuning::kSoftMaxReplyNodes));
+            for (auto const& nodeId : m->nodeids())
+            {
+                if (nodeIDs.size() >= Tuning::kSoftMaxReplyNodes)
+                {
+                    // Charge the peer for sending too many node IDs, but continue processing the
+                    // received node IDs up to the limit. If the request is legitimate then at least
+                    // they will get a response and won't have to resend these nodes in their next
+                    // request.
+                    peer->charge(
+                        Resource::kFeeModerateBurdenPeer, "TMGetLedger: too many node IDs");
+                    break;
+                }
+                auto parsed = deserializeSHAMapNodeID(nodeId);
+                if (!parsed)
+                {
+                    peer->charge(Resource::kFeeInvalidData, "TMGetLedger: Invalid node ID");
+                    return;
+                }
+                nodeIDs.push_back(std::move(*parsed));
+            }
+        }
+
+        peer->processLedgerRequest(m, std::move(nodeIDs));
     });
 }
 
@@ -3319,7 +3338,9 @@ PeerImp::getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const
 }
 
 void
-PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
+PeerImp::processLedgerRequest(
+    std::shared_ptr<protocol::TMGetLedger> const& m,
+    std::vector<SHAMapNodeID> nodeIDs)
 {
     // Do not resource charge a peer responding to a relay
     if (!m->has_requestcookie())
@@ -3404,26 +3425,23 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
     }
 
     // Add requested node data to reply
-    if (m->nodeids_size() > 0)
+    if (!nodeIDs.empty())
     {
         std::uint32_t const defaultDepth = isHighLatency() ? 2 : 1;
         auto const queryDepth{m->has_querydepth() ? m->querydepth() : defaultDepth};
 
         std::vector<std::pair<SHAMapNodeID, Blob>> data;
-
-        for (int i = 0;
-             i < m->nodeids_size() && ledgerData.nodes_size() < Tuning::kSoftMaxReplyNodes;
-             ++i)
+        data.reserve(Tuning::kSoftMaxReplyNodes);
+        for (auto const& nodeID : nodeIDs)
         {
-            auto const shaMapNodeId{deserializeSHAMapNodeID(m->nodeids(i))};
+            if (ledgerData.nodes_size() >= Tuning::kSoftMaxReplyNodes)
+                break;
 
             data.clear();
-            data.reserve(Tuning::kSoftMaxReplyNodes);
 
             try
             {
-                // NOLINTNEXTLINE(bugprone-unchecked-optional-access) nodeids checked in onGetLedger
-                if (map->getNodeFat(*shaMapNodeId, data, fatLeaves, queryDepth))
+                if (map->getNodeFat(nodeID, data, fatLeaves, queryDepth))
                 {
                     JLOG(pJournal_.trace())
                         << "processLedgerRequest: getNodeFat got " << data.size() << " nodes";
@@ -3432,9 +3450,9 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
                     {
                         if (ledgerData.nodes_size() >= Tuning::kHardMaxReplyNodes)
                             break;
-                        protocol::TMLedgerNode* node{ledgerData.add_nodes()};
-                        node->set_nodeid(d.first.getRawString());
-                        node->set_nodedata(d.second.data(), d.second.size());
+                        protocol::TMLedgerNode* ledgerNode{ledgerData.add_nodes()};
+                        ledgerNode->set_nodeid(d.first.getRawString());
+                        ledgerNode->set_nodedata(d.second.data(), d.second.size());
                     }
                 }
                 else
@@ -3473,14 +3491,14 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
                     info += ", no hash specified";
 
                 JLOG(pJournal_.warn())
-                    << "processLedgerRequest: getNodeFat with nodeId " << *shaMapNodeId
+                    << "processLedgerRequest: getNodeFat with nodeId " << nodeID
                     << " and ledger info type " << info << " throws exception: " << e.what();
             }
         }
 
         JLOG(pJournal_.info()) << "processLedgerRequest: Got request for " << m->nodeids_size()
-                               << " nodes at depth " << queryDepth << ", return "
-                               << ledgerData.nodes_size() << " nodes";
+                               << " node IDs (processed " << nodeIDs.size() << ") at depth "
+                               << queryDepth << ", return " << ledgerData.nodes_size() << " nodes";
     }
 
     if (ledgerData.nodes_size() == 0)

src/xrpld/overlay/detail/PeerImp.h

@@ -14,6 +14,7 @@
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/STValidation.h>
 #include <xrpl/resource/Fees.h>
+#include <xrpl/shamap/SHAMapNodeID.h>
 
 #include <boost/circular_buffer.hpp>
 #include <boost/endian/conversion.hpp>
@@ -629,7 +630,9 @@ private:
     getTxSet(std::shared_ptr<protocol::TMGetLedger> const& m) const;
 
     void
-    processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m);
+    processLedgerRequest(
+        std::shared_ptr<protocol::TMGetLedger> const& m,
+        std::vector<SHAMapNodeID> nodeIDs);
 
 protected:
     // Kept `protected` so test subclasses (see