perf: Move entries off NodePathStack instead of copying them

[bthomee]

Part 8/8 of a stack (top). Base: part 7 (bthomee/shamap-stack-07-nodepathstack-fail-closed).

Five sites copied the top entry out and then popped it. SHAMapTreeNodePtr is
refcounted, so each copy bumped its atomic strong-ref count; release() moves
the pointer out instead, a plain pointer swap with no atomic. SHAMapNodeID
also derives from CountedObject, but CountedObject declares no move
constructor, so moving a SHAMapNodeID still runs its copy constructor;
release() saves nothing on the ID half of the pair. dirtyUp and delItem
walk up to 64 levels per insert or delete on the ledger write path, so this
removes up to 64 atomic operations per call, not 128. The two sites that read
without popping now bind a reference rather than copying.

dirtyUp and delItem both drop a dynamicPointerCast/null check for a
static cast, on the reasoning that by the time either receives the stack,
addGiveItem/updateGiveItem have already consumed the terminal leaf entry
via release(), so every remaining entry is provably an inner node. That
reasoning holds today, but an XRPL_ASSERT is a no-op under NDEBUG, so
both get a real UNREACHABLE-guarded check instead: a release build that
somehow violated the invariant would otherwise write through a
misinterpreted node via setChild, silent memory corruption in place of the
clean crash dynamicPointerCast used to produce. updateGiveItem's own
cast needed the same treatment for a different reason: an absent tag leaves
an inner node on top of the stack, and the cast that followed the assertion
there would have reinterpreted an SHAMapInnerNode as a SHAMapLeafNode.
Replaced with if (!top->isLeaf()) return false;, pinned by a regression
test.

The std::move these sites previously applied to
staticPointerCast/dynamicPointerCast was dropped rather than fixed: both
only had a TT const& overload, so the move bound to that const ref and
copied anyway, silently defeating the SHAMapTreeNodePtr refcount saving
described above. Adds the missing rvalue overload to each, tied to
SharedIntrusive<TT>&& rather than a bare TT&& so it cannot also bind to
an lvalue in preference to the const-ref overload, and restores std::move
at the three call sites that own a soon-to-be-discarded pointer.

[Copilot]

Pull request overview

This PR reduces SHAMap traversal overhead by moving NodePathStack entries out (instead of copying then popping), avoiding unnecessary refcount churn on the ledger write path. It also hardens assumptions around inner/leaf node types with explicit runtime checks, and adds a regression test for updateGiveItem on absent keys.

Changes:

• Add NodePathStack::release() to pop-and-return the top entry via move, and update hot call sites to use it.
• Add rvalue overloads for intr_ptr::{static,dynamic}PointerCast so std::move actually enables move semantics for SharedIntrusive.
• Add a unit test verifying updateGiveItem returns false when the target key is absent (top-of-stack can be an inner node).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/tests/libxrpl/shamap/SHAMap.cpp	Adds regression coverage for updateGiveItem when walkTowardsKey ends on an inner node for an absent key.
src/libxrpl/shamap/SHAMap.cpp	Switches several stack consumers to release()/reference binding, adds non-debug-safe node-type checks, and restores real move semantics at ownership-discard sites.
include/xrpl/shamap/SHAMap.h	Introduces NodePathStack::release() and improves walkTowardsKey parameter documentation.
include/xrpl/basics/IntrusivePointer.h	Adds rvalue overloads for staticPointerCast/dynamicPointerCast to avoid unintended copies when moving SharedIntrusive.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.