refactor: Move entries off NodePathStack instead of copying them

Five sites copied the top entry out and then popped it. `SHAMapTreeNodePtr` is
refcounted, so each copy bumped the pointee's atomic strong count and the
original's destructor then released it. `releaseNode()` moves the pointer out
instead, a plain swap with no atomic at all. `dirtyUp` and `delItem` walk up to
64 levels per insert or delete on the ledger write path, so this removes up to 64
increments and 64 release sequences per call. The sites that also want the ID
read `top().second` first, which costs the same either way, and the two that read
without popping now bind a reference.

`staticPointerCast` and `dynamicPointerCast` had only a `TT const&` overload, so
no caller could move into them. Each gains an rvalue overload, tied to
`SharedIntrusive<TT>&&` rather than a bare `TT&&` so it cannot bind to an lvalue
in preference to the const-ref one, and the sites that own a discarded pointer
now pass `std::move`. `SharedIntrusive`'s move constructors also become
`noexcept`, so a `std::vector` of them relocates by moving; without that,
`move_if_noexcept` copies every element, since the type is copy constructible.

Three of the casts become static, and a fourth that already was gains the same
live type test, so no traversal path is left paying for a `dynamic_cast`.
`dirtyUp` and `delItem`'s loop rest on every remaining entry being inner, which
holds but was only an `XRPL_ASSERT`, a no-op under `NDEBUG`, so both report
`UNREACHABLE` and throw rather than writing through a misread node.
`updateGiveItem` and `delItem`'s leaf cast need the test for a different reason:
an absent tag leaves an inner node on top, which the public API permits, so they
return false rather than aborting an instrumented build. A test pins that.

include/xrpl/basics/IntrusivePointer.h

@@ -86,12 +86,16 @@ public:
         requires std::convertible_to<TT*, T*>
     SharedIntrusive(SharedIntrusive<TT> const& rhs);
 
-    SharedIntrusive(SharedIntrusive&& rhs);
+    // noexcept so that a std::vector of these relocates by moving. Without it, move_if_noexcept
+    // copies each element instead, since this type is also copy constructible, and every copy is an
+    // atomic increment on the pointee's refcount followed by a release on the original. The body is
+    // a std::exchange on a raw pointer, so it provably cannot throw.
+    SharedIntrusive(SharedIntrusive&& rhs) noexcept;
 
     template <class TT>
         requires std::convertible_to<TT*, T*>
-    SharedIntrusive(
-        SharedIntrusive<TT>&& rhs);  // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved)
+    // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved)
+    SharedIntrusive(SharedIntrusive<TT>&& rhs) noexcept;
 
     SharedIntrusive&
     operator=(SharedIntrusive const& rhs);
@@ -529,11 +533,48 @@ staticPointerCast(TT const& v)
     return SharedPtr<T>(StaticCastTagSharedIntrusive{}, v);
 }
 
+/**
+ * Statically cast an intrusive pointer the caller is giving up, moving out of
+ * it.
+ *
+ * The parameter names the wrapped type rather than taking a bare `TT&&`. A
+ * bare one would be a forwarding reference, so it would also bind to lvalues
+ * in preference to the `const&` overload above and move out of a caller's live
+ * variable on what looks like a copy call.
+ *
+ * @param v the pointer to cast, left empty afterwards.
+ * @return a pointer of the requested type to the same object.
+ */
+template <class T, class TT>
+SharedPtr<T>
+staticPointerCast(SharedIntrusive<TT>&& v)
+{
+    return SharedPtr<T>(StaticCastTagSharedIntrusive{}, std::move(v));
+}
+
 template <class T, class TT>
 SharedPtr<T>
 dynamicPointerCast(TT const& v)
 {
     return SharedPtr<T>(DynamicCastTagSharedIntrusive{}, v);
 }
+
+/**
+ * Dynamically cast an intrusive pointer the caller is giving up, moving out of
+ * it.
+ *
+ * Tied to `SharedIntrusive<TT>&&` for the reason given above.
+ *
+ * @param v the pointer to cast, left empty afterwards if the cast succeeds and
+ *          left owning the object if it does not.
+ * @return a pointer of the requested type, or an empty one if the object is
+ *         not of that type.
+ */
+template <class T, class TT>
+SharedPtr<T>
+dynamicPointerCast(SharedIntrusive<TT>&& v)
+{
+    return SharedPtr<T>(DynamicCastTagSharedIntrusive{}, std::move(v));
+}
 }  // namespace intr_ptr
 }  // namespace xrpl

include/xrpl/basics/IntrusivePointer.ipp

@@ -43,7 +43,7 @@ SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT> const& rhs)
 }
 
 template <class T>
-SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs)
+SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs) noexcept
     : ptr_{std::move(rhs).unsafeExchange(nullptr)}
 {
 }
@@ -51,7 +51,7 @@ SharedIntrusive<T>::SharedIntrusive(SharedIntrusive&& rhs)
 template <class T>
 template <class TT>
     requires std::convertible_to<TT*, T*>
-SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT>&& rhs)
+SharedIntrusive<T>::SharedIntrusive(SharedIntrusive<TT>&& rhs) noexcept
     : ptr_{std::move(rhs).unsafeExchange(nullptr)}
 {
 }

include/xrpl/shamap/SHAMap.h

@@ -519,6 +519,33 @@ private:
             stack_ = {};
         }
 
+        /**
+         * Shorten the path by one node and hand that node to the caller,
+         * keeping its ID.
+         *
+         * Reading a node out and then popping copies it, which costs an atomic
+         * increment on its refcount. Moving it out does not. A caller that
+         * wants the ID as well reads `top().second` first, which costs the
+         * same either way: `SHAMapNodeID` declares no move constructor.
+         *
+         * @return the node that was at the end of the path, or an empty
+         *         pointer if there was none.
+         */
+        [[nodiscard]] SHAMapTreeNodePtr
+        releaseNode()
+        {
+            if (stack_.empty())
+            {
+                // LCOV_EXCL_START
+                UNREACHABLE("xrpl::SHAMap::NodePathStack::releaseNode : empty stack");
+                return {};
+                // LCOV_EXCL_STOP
+            }
+            auto node = std::move(stack_.top().first);
+            stack_.pop();
+            return node;
+        }
+
         /**
          * Start a path at the root of the map, whose ID is the zero-depth ID by definition.
          *
@@ -655,9 +682,15 @@ private:
     dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr terminal);
 
     /**
-     * Walk towards the specified id, returning the node.  Caller must check
-     *  if the return is nullptr, and if not, if the node->peekItem()->key() ==
-     * id
+     * Walk towards the specified id, returning the node.
+     *
+     * @param id the key to walk towards, which need not be in the map.
+     * @param stack records the path walked, or nullptr to skip recording it.
+     *              Lookups that only want the leaf (see findKey) omit it to
+     *              avoid building a path they would immediately discard.
+     * @return the leaf the walk ended on, or nullptr if it ended on an inner
+     *         node or was refused. A returned leaf need not hold `id`, so
+     *         callers compare its key themselves.
      */
     SHAMapLeafNode*
     walkTowardsKey(uint256 const& id, NodePathStack* stack = nullptr) const;

src/libxrpl/shamap/SHAMap.cpp

@@ -111,10 +111,16 @@ SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr c
 
     while (!stack.empty())
     {
-        auto node = intr_ptr::dynamicPointerCast<SHAMapInnerNode>(stack.top().first);
-        SHAMapNodeID const nodeID = stack.top().second;
-        stack.pop();
-        XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node");
+        auto const nodeID = stack.top().second;
+        auto top = stack.releaseNode();
+        if (!top->isInner())
+        {
+            // LCOV_EXCL_START
+            UNREACHABLE("xrpl::SHAMap::dirtyUp : node is not inner");
+            Throw<SHAMapMissingNode>(type_, target);
+            // LCOV_EXCL_STOP
+        }
+        auto node = intr_ptr::staticPointerCast<SHAMapInnerNode>(std::move(top));
 
         auto const branch = selectBranch(nodeID, target);
 
@@ -621,7 +627,7 @@ SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const
     stack.pop();
     while (!stack.empty())
     {
-        auto const [node, nodeID] = stack.top();
+        auto const& [node, nodeID] = stack.top();
         XRPL_ASSERT(!node->isLeaf(), "xrpl::SHAMap::peekNextItem : another node is not leaf");
         auto& inner = safeDowncast<SHAMapInnerNode&>(*node);
         for (auto i = selectBranch(nodeID, id) + 1; i < kBranchFactor; ++i)
@@ -690,7 +696,7 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const
 
     while (!stack.empty())
     {
-        auto const [node, nodeID] = stack.top();
+        auto const& [node, nodeID] = stack.top();
         if (node->isLeaf())
         {
             auto const& item = safeDowncast<SHAMapLeafNode const&>(*node).peekItem();
@@ -756,10 +762,15 @@ SHAMap::delItem(uint256 const& id)
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, id);
 
-    auto leaf = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
-    stack.pop();
+    // An absent id leaves an inner node on top rather than a leaf, which is a "not found" answer
+    // and not a fault, so it is tested rather than cast through. Matches the three sibling sites in
+    // this commit, so no traversal path is left paying for a dynamic_cast.
+    auto top = stack.releaseNode();
+    if (!top->isLeaf())
+        return false;
+    auto leaf = intr_ptr::staticPointerCast<SHAMapLeafNode>(std::move(top));
 
-    if (!leaf || (leaf->peekItem()->key() != id))
+    if (leaf->peekItem()->key() != id)
         return false;
 
     SHAMapNodeType const type = leaf->getType();
@@ -769,9 +780,16 @@ SHAMap::delItem(uint256 const& id)
 
     while (!stack.empty())
     {
-        auto node = intr_ptr::staticPointerCast<SHAMapInnerNode>(stack.top().first);
-        SHAMapNodeID const nodeID = stack.top().second;
-        stack.pop();
+        auto const nodeID = stack.top().second;
+        auto top = stack.releaseNode();
+        if (!top->isInner())
+        {
+            // LCOV_EXCL_START
+            UNREACHABLE("xrpl::SHAMap::delItem : node is not inner");
+            Throw<SHAMapMissingNode>(type_, id);
+            // LCOV_EXCL_STOP
+        }
+        auto node = intr_ptr::staticPointerCast<SHAMapInnerNode>(std::move(top));
 
         node = unshareNode(std::move(node), nodeID);
         node->setChild(
@@ -842,8 +860,8 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, tag);
 
-    auto [node, nodeID] = stack.top();
-    stack.pop();
+    auto nodeID = stack.top().second;
+    auto node = stack.releaseNode();
 
     if (node->isLeaf())
     {
@@ -938,16 +956,26 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem cons
     if (stack.empty())
         Throw<SHAMapMissingNode>(type_, tag);
 
-    auto node = intr_ptr::dynamicPointerCast<SHAMapLeafNode>(stack.top().first);
-    auto nodeID = stack.top().second;
-    stack.pop();
+    auto const nodeID = stack.top().second;
+    auto top = stack.releaseNode();
 
-    if (!node || (node->peekItem()->key() != tag))
+    // walkTowardsKey pushes an inner node's own entry before testing whether the branch it needs
+    // is empty, so a tag absent from the map leaves that inner node on top rather than a leaf.
+    // No in-tree caller reaches this, since each checks the item exists first, but the API is
+    // public and permits the call, which is why it returns false rather than reporting UNREACHABLE.
+    // The static cast below is also safe only once this is confirmed.
+    if (!top->isLeaf())
+    {
+        return false;
+    }
+    auto node = intr_ptr::staticPointerCast<SHAMapLeafNode>(std::move(top));
+
+    // The other shape an absent tag takes: the walk ends on the leaf it reached, whose key need not
+    // be `tag`, which is why findKey discards such a leaf. Both shapes mean the same thing to a
+    // caller, so both answer false rather than reporting UNREACHABLE.
+    if (node->peekItem()->key() != tag)
     {
-        // LCOV_EXCL_START
-        UNREACHABLE("xrpl::SHAMap::updateGiveItem : invalid node");
         return false;
-        // LCOV_EXCL_STOP
     }
 
     if (node->getType() != type)

src/tests/libxrpl/shamap/SHAMap.cpp

@@ -459,6 +459,39 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys)
     }
 }
 
+TEST_F(SHAMapTraversal, update_give_item_on_absent_key_returns_false)
+{
+    tests::TestNodeFamily f{j_};
+    auto keys = deepFanOutKeys();
+    SHAMap map{SHAMapType::FREE, f};
+    fillMap(map, keys);
+
+    // Absent key: walkTowardsKey stops on an inner node with an empty branch, not a leaf, so
+    // updateGiveItem has to answer for a top of the wrong type. The public API permits the call,
+    // so it returns false rather than reporting UNREACHABLE and aborting an instrumented build.
+    auto const absentKey = uint256{std::string_view{std::string(64, '0')}};
+    Buffer vuc{32};
+    std::fill_n(vuc.data(), vuc.size(), std::uint8_t{2});
+    EXPECT_FALSE(map.updateGiveItem(
+        SHAMapNodeType::TnAccountState, makeShamapitem(absentKey, std::move(vuc))));
+
+    // The other shape an absent key takes: a single-item map queried with a key that selects the
+    // same root branch. The walk ends on the leaf it reached, whose key is not the one asked for,
+    // so the top is a leaf that does not hold the tag. Both shapes answer false. The difference
+    // only shows in an instrumented build, where reporting UNREACHABLE would abort.
+    SHAMap single{SHAMapType::FREE, f};
+    fillMap(single, {keys.front()});
+
+    auto probe = keys.front();
+    std::fill_n(probe.begin() + 1, probe.size() - 1, std::uint8_t{0});
+    ASSERT_NE(probe, keys.front());
+
+    Buffer other{32};
+    std::fill_n(other.data(), other.size(), std::uint8_t{3});
+    EXPECT_FALSE(single.updateGiveItem(
+        SHAMapNodeType::TnAccountState, makeShamapitem(probe, std::move(other))));
+}
+
 TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end)
 {
     tests::TestNodeFamily f{j_};