From 5337d028a2559bd75ec46b60b7a5539487d18d0a Mon Sep 17 00:00:00 2001 From: Bart Date: Mon, 17 Aug 2026 10:07:14 +0000 Subject: [PATCH 1/5] refactor: Use unsigned int for branch-related operations (#7938) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 --- include/xrpl/shamap/SHAMap.h | 35 +++++---- include/xrpl/shamap/SHAMapInnerNode.h | 26 +++---- include/xrpl/shamap/SHAMapNodeID.h | 4 +- include/xrpl/shamap/detail/TaggedPointer.h | 12 +-- include/xrpl/shamap/detail/TaggedPointer.ipp | 59 +++++++------- src/libxrpl/shamap/SHAMap.cpp | 77 +++++++++---------- src/libxrpl/shamap/SHAMapDelta.cpp | 22 +++--- src/libxrpl/shamap/SHAMapInnerNode.cpp | 69 ++++++++--------- src/libxrpl/shamap/SHAMapNodeID.cpp | 15 ++-- src/libxrpl/shamap/SHAMapSync.cpp | 58 ++++++++------ .../app/ledger/detail/LedgerNodeHelpers.cpp | 2 +- 11 files changed, 195 insertions(+), 184 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 97ab2e9f7a..05de33ddf3 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -484,31 +484,36 @@ private: // returns the first item at or below this node SHAMapLeafNode* - firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch = 0) const; + firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch = 0u) const; // returns the last item at or below this node SHAMapLeafNode* - lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch = kBranchFactor) const; + lastBelow( + SHAMapTreeNodePtr node, + SharedPtrNodeStack& stack, + unsigned int branch = kBranchFactor) const; + + // direction in which belowHelper scans an inner node's branches + enum class BelowDirection { First, Last }; // helper function for firstBelow and lastBelow SHAMapLeafNode* belowHelper( SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, - int branch, - std::tuple, std::function> const& loopParams) - const; + unsigned int branch, + BelowDirection direction) const; // Simple descent // Get a child of the specified node SHAMapTreeNode* - descend(SHAMapInnerNode*, int branch) const; + descend(SHAMapInnerNode*, unsigned int branch) const; SHAMapTreeNode* - descendThrow(SHAMapInnerNode*, int branch) const; + descendThrow(SHAMapInnerNode*, unsigned int branch) const; SHAMapTreeNodePtr - descend(SHAMapInnerNode&, int branch) const; + descend(SHAMapInnerNode&, unsigned int branch) const; SHAMapTreeNodePtr - descendThrow(SHAMapInnerNode&, int branch) const; + descendThrow(SHAMapInnerNode&, unsigned int branch) const; // Descend with filter // If pending, callback is called as if it called fetchNodeNT @@ -516,7 +521,7 @@ private: SHAMapTreeNode* descendAsync( SHAMapInnerNode* parent, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter, bool& pending, descendCallback&&) const; @@ -525,13 +530,13 @@ private: descend( SHAMapInnerNode* parent, SHAMapNodeID const& parentID, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter) const; // Non-storing // Does not hook the returned node to its parent SHAMapTreeNodePtr - descendNoStore(SHAMapInnerNode&, int branch) const; + descendNoStore(SHAMapInnerNode&, unsigned int branch) const; /** * If there is only one leaf below this node, get its contents @@ -581,8 +586,8 @@ private: using StackEntry = std::tuple< SHAMapInnerNode*, // pointer to the node SHAMapNodeID, // the node's ID - int, // while child we check first - int, // which child we check next + unsigned int, // which child we check first + unsigned int, // which child we check next bool>; // whether we've found any missing children yet // We explicitly choose to specify the use of std::deque here, because @@ -596,7 +601,7 @@ private: using DeferredNode = std::tuple< SHAMapInnerNode*, // parent node SHAMapNodeID, // parent node ID - int, // branch + unsigned int, // branch SHAMapTreeNodePtr>; // node int deferred; diff --git a/include/xrpl/shamap/SHAMapInnerNode.h b/include/xrpl/shamap/SHAMapInnerNode.h index 44d3bd6279..83d039172f 100644 --- a/include/xrpl/shamap/SHAMapInnerNode.h +++ b/include/xrpl/shamap/SHAMapInnerNode.h @@ -62,8 +62,8 @@ private: * * @param i index of the requested child */ - std::optional - getChildIndex(int i) const; + std::optional + getChildIndex(unsigned int i) const; /** * Call the `f` callback for all 16 (branchFactor) branches - even if @@ -125,28 +125,28 @@ public: isEmpty() const; bool - isEmptyBranch(int m) const; + isEmptyBranch(unsigned int branch) const; - int + unsigned int getBranchCount() const; SHAMapHash const& - getChildHash(int m) const; + getChildHash(unsigned int branch) const; void - setChild(int m, SHAMapTreeNodePtr child); + setChild(unsigned int branch, SHAMapTreeNodePtr child); void - shareChild(int m, SHAMapTreeNodePtr const& child); + shareChild(unsigned int branch, SHAMapTreeNodePtr const& child); SHAMapTreeNode* - getChildPointer(int branch); + getChildPointer(unsigned int branch); SHAMapTreeNodePtr - getChild(int branch); + getChild(unsigned int branch); SHAMapTreeNodePtr - canonicalizeChild(int branch, SHAMapTreeNodePtr node); + canonicalizeChild(unsigned int branch, SHAMapTreeNodePtr node); // sync functions bool @@ -190,12 +190,12 @@ SHAMapInnerNode::isEmpty() const } inline bool -SHAMapInnerNode::isEmptyBranch(int m) const +SHAMapInnerNode::isEmptyBranch(unsigned int branch) const { - return (isBranch_ & (1 << m)) == 0; + return (isBranch_ & (1u << branch)) == 0u; } -inline int +inline unsigned int SHAMapInnerNode::getBranchCount() const { return popcnt16(isBranch_); diff --git a/include/xrpl/shamap/SHAMapNodeID.h b/include/xrpl/shamap/SHAMapNodeID.h index 1189304aa7..fcd5a4d00e 100644 --- a/include/xrpl/shamap/SHAMapNodeID.h +++ b/include/xrpl/shamap/SHAMapNodeID.h @@ -53,7 +53,7 @@ public: } [[nodiscard]] SHAMapNodeID - getChildNodeID(unsigned int m) const; + getChildNodeID(unsigned int branch) const; /** * Create a SHAMapNodeID of a node with the depth of the node and @@ -64,7 +64,7 @@ public: * @return SHAMapNodeID of the node */ static SHAMapNodeID - createID(int depth, uint256 const& key); + createID(unsigned int depth, uint256 const& key); /** * Comparison operators diff --git a/include/xrpl/shamap/detail/TaggedPointer.h b/include/xrpl/shamap/detail/TaggedPointer.h index 509e6cc58d..705681be1d 100644 --- a/include/xrpl/shamap/detail/TaggedPointer.h +++ b/include/xrpl/shamap/detail/TaggedPointer.h @@ -219,11 +219,11 @@ public: * * @param i index of the requested child */ - [[nodiscard]] std::optional - getChildIndex(std::uint16_t isBranch, int i) const; + [[nodiscard]] std::optional + getChildIndex(std::uint16_t isBranch, unsigned int i) const; }; -[[nodiscard]] inline int +[[nodiscard]] inline unsigned int popcnt16(std::uint16_t a) { #if __cpp_lib_bitops @@ -234,11 +234,11 @@ popcnt16(std::uint16_t a) // fallback to table lookup static constexpr auto tbl = []() { std::array ret{}; - for (int i = 0; i != 256; ++i) + for (auto i = 0u; i != 256u; ++i) { - for (int j = 0; j != 8; ++j) + for (auto j = 0u; j != 8u; ++j) { - if (i & (1 << j)) + if (i & (1u << j)) ret[i]++; } } diff --git a/include/xrpl/shamap/detail/TaggedPointer.ipp b/include/xrpl/shamap/detail/TaggedPointer.ipp index 9275f3d15a..7db101b3cb 100644 --- a/include/xrpl/shamap/detail/TaggedPointer.ipp +++ b/include/xrpl/shamap/detail/TaggedPointer.ipp @@ -22,6 +22,11 @@ static_assert( static_assert( kBoundaries.back() == SHAMapInnerNode::kBranchFactor, "Last element of boundaries must be number of children in a dense array"); +static_assert( + kBoundaries.front() >= 1, + "TaggedPointer.ipp subtracts 1 from a numAllocated value derived from " + "kBoundaries, as an unsigned quantity, in several places; the smallest " + "boundary must stay non-zero or those subtractions underflow."); // Terminology: A chunk is the memory being allocated from a block. A block // contains multiple chunks. This is the terminology the boost documentation @@ -148,16 +153,16 @@ TaggedPointer::iterChildren(std::uint16_t isBranch, F&& f) const if (numAllocated == SHAMapInnerNode::kBranchFactor) { // dense case - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) f(hashes[i]); } else { // sparse case - int curHashI = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto curHashI = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(hashes[curHashI++]); } @@ -176,9 +181,9 @@ TaggedPointer::iterNonEmptyChildIndexes(std::uint16_t isBranch, F&& f) const if (capacity() == SHAMapInnerNode::kBranchFactor) { // dense case - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(i, i); } @@ -187,10 +192,10 @@ TaggedPointer::iterNonEmptyChildIndexes(std::uint16_t isBranch, F&& f) const else { // sparse case - int curHashI = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto curHashI = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if ((1 << i) & isBranch) + if ((1u << i) & isBranch) { f(i, curHashI++); } @@ -216,14 +221,14 @@ TaggedPointer::destroyHashesAndChildren() deallocateArrays(tag, ptr); } -inline std::optional -TaggedPointer::getChildIndex(std::uint16_t isBranch, int i) const +inline std::optional +TaggedPointer::getChildIndex(std::uint16_t isBranch, unsigned int i) const { if (isDense()) return i; // Sparse case - if ((isBranch & (1 << i)) == 0) + if ((isBranch & (1u << i)) == 0u) { // Empty branch. Sparse children do not store empty branches return {}; @@ -273,10 +278,10 @@ inline TaggedPointer::TaggedPointer( *this = std::move(other); auto [srcDstNumAllocated, srcDstHashes, srcDstChildren] = getHashesAndChildren(); bool const srcDstIsDense = isDense(); - int srcDstIndex = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto srcDstIndex = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - auto const mask = (1 << i); + auto const mask = (1u << i); bool const inSrc = (srcBranches & mask) != 0; bool const inDst = (dstBranches & mask) != 0; if (inSrc && inDst) @@ -298,13 +303,13 @@ inline TaggedPointer::TaggedPointer( // sparse // need to shift all the elements to the left by // one - for (int c = srcDstIndex; c < srcDstNumAllocated - 1; ++c) + for (auto c = srcDstIndex; c + 1 < srcDstNumAllocated; ++c) { srcDstHashes[c] = srcDstHashes[c + 1]; srcDstChildren[c] = std::move(srcDstChildren[c + 1]); } - srcDstHashes[srcDstNumAllocated - 1].zero(); - srcDstChildren[srcDstNumAllocated - 1].reset(); + srcDstHashes[srcDstNumAllocated - 1u].zero(); + srcDstChildren[srcDstNumAllocated - 1u].reset(); // do not increment the index } } @@ -321,7 +326,7 @@ inline TaggedPointer::TaggedPointer( // sparse // need to create a hole by shifting all the elements to the // right by one - for (int c = srcDstNumAllocated - 1; c > srcDstIndex; --c) + for (auto c = srcDstNumAllocated - 1u; c > srcDstIndex; --c) { srcDstHashes[c] = srcDstHashes[c - 1]; srcDstChildren[c] = std::move(srcDstChildren[c - 1]); @@ -352,10 +357,10 @@ inline TaggedPointer::TaggedPointer( auto [srcNumAllocated, srcHashes, srcChildren] = src.getHashesAndChildren(); bool const srcIsDense = src.isDense(); bool const dstIsDense = dst.isDense(); - int srcIndex = 0, dstIndex = 0; - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + auto srcIndex = 0u, dstIndex = 0u; + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - auto const mask = (1 << i); + auto const mask = (1u << i); bool const inSrc = (srcBranches & mask) != 0; bool const inDst = (dstBranches & mask) != 0; if (inSrc && inDst) @@ -409,7 +414,7 @@ inline TaggedPointer::TaggedPointer( !dstIsDense || dstIndex == dstNumAllocated, "xrpl::TaggedPointer::TaggedPointer(TaggedPointer&& ...) : " "non-sparse or valid sparse"); - for (int i = dstIndex; i < dstNumAllocated; ++i) + for (auto i = dstIndex; i < dstNumAllocated; ++i) { new (&dstHashes[i]) SHAMapHash{}; new (&dstChildren[i]) SHAMapTreeNodePtr{}; @@ -448,9 +453,9 @@ inline TaggedPointer::TaggedPointer( new (&newChildren[branchNum]) SHAMapTreeNodePtr{std::move(oldChildren[indexNum])}; }); // Run the constructors for the remaining elements - for (int i = 0; i < SHAMapInnerNode::kBranchFactor; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { - if (((1 << i) & isBranch) != 0) + if (((1u << i) & isBranch) != 0u) continue; new (&newHashes[i]) SHAMapHash{}; new (&newChildren[i]) SHAMapTreeNodePtr{}; @@ -459,7 +464,7 @@ inline TaggedPointer::TaggedPointer( else { // new arrays are sparse, old arrays may be sparse or dense - int curCompressedIndex = 0; + auto curCompressedIndex = 0u; iterNonEmptyChildIndexes(isBranch, [&](auto branchNum, auto indexNum) { new (&newHashes[curCompressedIndex]) SHAMapHash{oldHashes[indexNum]}; new (&newChildren[curCompressedIndex]) @@ -467,7 +472,7 @@ inline TaggedPointer::TaggedPointer( ++curCompressedIndex; }); // Run the constructors for the remaining elements - for (int i = curCompressedIndex; i < newNumAllocated; ++i) + for (auto i = curCompressedIndex; i < newNumAllocated; ++i) { new (&newHashes[i]) SHAMapHash{}; new (&newChildren[i]) SHAMapTreeNodePtr{}; diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 2483e6f6e1..3fa8d66be0 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -116,8 +116,7 @@ SHAMap::dirtyUp(SharedPtrNodeStack& stack, uint256 const& target, SHAMapTreeNode stack.pop(); XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node"); - int const branch = selectBranch(nodeID, target); - XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::dirtyUp : valid branch"); + auto const branch = selectBranch(nodeID, target); node = unshareNode(std::move(node), nodeID); node->setChild(branch, std::move(child)); @@ -278,7 +277,7 @@ SHAMap::fetchNode(SHAMapHash const& hash) const } SHAMapTreeNode* -SHAMap::descendThrow(SHAMapInnerNode* parent, int branch) const +SHAMap::descendThrow(SHAMapInnerNode* parent, unsigned int branch) const { SHAMapTreeNode* ret = descend(parent, branch); // NOLINT(misc-const-correctness) @@ -289,7 +288,7 @@ SHAMap::descendThrow(SHAMapInnerNode* parent, int branch) const } SHAMapTreeNodePtr -SHAMap::descendThrow(SHAMapInnerNode& parent, int branch) const +SHAMap::descendThrow(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = descend(parent, branch); @@ -300,7 +299,7 @@ SHAMap::descendThrow(SHAMapInnerNode& parent, int branch) const } SHAMapTreeNode* -SHAMap::descend(SHAMapInnerNode* parent, int branch) const +SHAMap::descend(SHAMapInnerNode* parent, unsigned int branch) const { SHAMapTreeNode* ret = parent->getChildPointer(branch); // NOLINT(misc-const-correctness) if ((ret != nullptr) || !backed_) @@ -315,7 +314,7 @@ SHAMap::descend(SHAMapInnerNode* parent, int branch) const } SHAMapTreeNodePtr -SHAMap::descend(SHAMapInnerNode& parent, int branch) const +SHAMap::descend(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr node = parent.getChild(branch); if (node || !backed_) @@ -332,7 +331,7 @@ SHAMap::descend(SHAMapInnerNode& parent, int branch) const // Gets the node that would be hooked to this branch, // but doesn't hook it up. SHAMapTreeNodePtr -SHAMap::descendNoStore(SHAMapInnerNode& parent, int branch) const +SHAMap::descendNoStore(SHAMapInnerNode& parent, unsigned int branch) const { SHAMapTreeNodePtr ret = parent.getChild(branch); if (!ret && backed_) @@ -344,12 +343,11 @@ std::pair SHAMap::descend( SHAMapInnerNode* parent, SHAMapNodeID const& parentID, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter) const { XRPL_ASSERT(parent->isInner(), "xrpl::SHAMap::descend : valid parent input"); - XRPL_ASSERT( - (branch >= 0) && (branch < kBranchFactor), "xrpl::SHAMap::descend : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMap::descend : valid branch input"); XRPL_ASSERT( !parent->isEmptyBranch(branch), "xrpl::SHAMap::descend : parent branch is non-empty"); @@ -373,7 +371,7 @@ SHAMap::descend( SHAMapTreeNode* SHAMap::descendAsync( SHAMapInnerNode* parent, - int branch, + unsigned int branch, SHAMapSyncFilter const* filter, bool& pending, descendCallback&& callback) const @@ -433,10 +431,9 @@ SHAMapLeafNode* SHAMap::belowHelper( SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, - int branch, - std::tuple, std::function> const& loopParams) const + unsigned int branch, + BelowDirection direction) const { - auto& [init, cmp, incr] = loopParams; if (node->isLeaf()) { auto n = intr_ptr::staticPointerCast(node); @@ -452,11 +449,16 @@ SHAMap::belowHelper( { stack.emplace(inner, stack.top().second.getChildNodeID(branch)); } - for (int i = init; cmp(i);) + // `scanned` counts how many branches of `inner` we have examined; the branch we look at is + // derived from it, so no index ever goes out of range. + for (auto scanned = 0u; scanned < kBranchFactor;) { - if (!inner->isEmptyBranch(i)) + auto const childBranch = + (direction == BelowDirection::Last) ? (kBranchFactor - 1u - scanned) : scanned; + + if (!inner->isEmptyBranch(childBranch)) { - node.adopt(descendThrow(inner.get(), i)); + node.adopt(descendThrow(inner.get(), childBranch)); XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack"); if (node->isLeaf()) { @@ -466,32 +468,24 @@ SHAMap::belowHelper( } inner = intr_ptr::staticPointerCast(node); stack.emplace(inner, stack.top().second.getChildNodeID(branch)); - i = init; // descend and reset loop + scanned = 0u; // descend and restart the scan on the new node } else { - incr(i); // scan next branch + ++scanned; // scan next branch } } return nullptr; } SHAMapLeafNode* -SHAMap::lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch) const +SHAMap::lastBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const { - auto init = kBranchFactor - 1; - auto cmp = [](int i) { return i >= 0; }; - auto incr = [](int& i) { --i; }; - - return belowHelper(node, stack, branch, {init, cmp, incr}); + return belowHelper(node, stack, branch, BelowDirection::Last); } SHAMapLeafNode* -SHAMap::firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, int branch) const +SHAMap::firstBelow(SHAMapTreeNodePtr node, SharedPtrNodeStack& stack, unsigned int branch) const { - auto init = 0; - auto cmp = [](int i) { return i <= kBranchFactor; }; - auto incr = [](int& i) { ++i; }; - - return belowHelper(node, stack, branch, {init, cmp, incr}); + return belowHelper(node, stack, branch, BelowDirection::First); } static boost::intrusive_ptr const kNoItem; @@ -504,7 +498,7 @@ SHAMap::onlyBelow(SHAMapTreeNode* node) const { SHAMapTreeNode* nextNode = nullptr; auto inner = safeDowncast(node); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { @@ -650,8 +644,9 @@ SHAMap::lowerBound(uint256 const& id) const else { auto inner = intr_ptr::staticPointerCast(node); - for (int branch = selectBranch(nodeID, id) - 1; branch >= 0; --branch) + for (auto branch = selectBranch(nodeID, id); branch > 0u;) { + --branch; if (!inner->isEmptyBranch(branch)) { node = descendThrow(*inner, branch); @@ -715,7 +710,7 @@ SHAMap::delItem(uint256 const& id) { // we may have made this a node with 1 or 0 children // And, if so, we need to remove this branch - int const bc = node->getBranchCount(); + auto const bc = node->getBranchCount(); if (bc == 0) { // no children below this branch @@ -730,7 +725,7 @@ SHAMap::delItem(uint256 const& id) if (item) { - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -786,7 +781,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr { // easy case, we end on an inner node auto inner = intr_ptr::staticPointerCast(node); - int const branch = selectBranch(nodeID, tag); + auto const branch = selectBranch(nodeID, tag); XRPL_ASSERT( inner->isEmptyBranch(branch), "xrpl::SHAMap::addGiveItem : inner branch is empty"); inner->setChild(branch, makeTypedLeaf(type, std::move(item), cowid_)); @@ -802,7 +797,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr node = intr_ptr::makeShared(node->cowid()); - unsigned int b1 = 0, b2 = 0; + auto b1 = 0u, b2 = 0u; while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key()))) { @@ -1012,12 +1007,12 @@ SHAMap::walkSubTree(bool doWrite, NodeObjectType t) // Stack of {parent,index,child} pointers representing // inner nodes we are in the process of flushing - using StackEntry = std::pair, int>; + using StackEntry = std::pair, unsigned int>; std::stack> stack; node = preFlushNode(std::move(node)); - int pos = 0; + auto pos = 0u; // We can't flush an inner node until we flush its children while (true) @@ -1032,7 +1027,7 @@ SHAMap::walkSubTree(bool doWrite, NodeObjectType t) { // No need to do I/O. If the node isn't linked, // it can't need to be flushed - int const branch = pos; + auto const branch = pos; auto child = node->getChild(pos++); if (child && (child->cowid() != 0)) @@ -1126,7 +1121,7 @@ SHAMap::dump(bool hash) const if (node->isInner()) { auto inner = safeDowncast(node); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { diff --git a/src/libxrpl/shamap/SHAMapDelta.cpp b/src/libxrpl/shamap/SHAMapDelta.cpp index 8336ce5481..1306fe6990 100644 --- a/src/libxrpl/shamap/SHAMapDelta.cpp +++ b/src/libxrpl/shamap/SHAMapDelta.cpp @@ -54,7 +54,7 @@ SHAMap::walkBranch( { // This is an inner node, add all non-empty branches auto inner = safeDowncast(node); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) nodeStack.push({descendThrow(inner, i)}); @@ -205,7 +205,7 @@ SHAMap::compare(SHAMap const& otherMap, Delta& differences, int maxCount) const { auto ours = safeDowncast(ourNode); auto other = safeDowncast(otherNode); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (ours->getChildHash(i) != other->getChildHash(i)) { @@ -257,7 +257,7 @@ SHAMap::walkMap(std::vector& missingNodes, int maxMissing) co intr_ptr::SharedPtr const node = std::move(nodeStack.top()); nodeStack.pop(); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -286,27 +286,29 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis return false; using StackEntry = intr_ptr::SharedPtr; - std::array topChildren; + std::array topChildren; { auto const& innerRoot = intr_ptr::staticPointerCast(root_); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (!innerRoot->isEmptyBranch(i)) topChildren[i] = descendNoStore(*innerRoot, i); } } std::vector workers; - workers.reserve(16); + workers.reserve(SHAMapInnerNode::kBranchFactor); std::vector exceptions; - exceptions.reserve(16); + exceptions.reserve(SHAMapInnerNode::kBranchFactor); - std::array>, 16> nodeStacks; + std::array>, SHAMapInnerNode::kBranchFactor> + nodeStacks; // This mutex is used inside the worker threads to protect `missingNodes` // and `maxMissing` from race conditions std::mutex m; - for (int rootChildIndex = 0; rootChildIndex < 16; ++rootChildIndex) + for (auto rootChildIndex = 0u; rootChildIndex < SHAMapInnerNode::kBranchFactor; + ++rootChildIndex) { auto const& child = topChildren[rootChildIndex]; if (!child || !child->isInner()) @@ -327,7 +329,7 @@ SHAMap::walkMapParallel(std::vector& missingNodes, int maxMis XRPL_ASSERT(node, "xrpl::SHAMap::walkMapParallel : non-null node"); nodeStack.pop(); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < SHAMapInnerNode::kBranchFactor; ++i) { if (node->isEmptyBranch(i)) continue; diff --git a/src/libxrpl/shamap/SHAMapInnerNode.cpp b/src/libxrpl/shamap/SHAMapInnerNode.cpp index 74a0e4515f..bdd89388b2 100644 --- a/src/libxrpl/shamap/SHAMapInnerNode.cpp +++ b/src/libxrpl/shamap/SHAMapInnerNode.cpp @@ -63,8 +63,8 @@ SHAMapInnerNode::resizeChildArrays(std::uint8_t toAllocate) hashesAndChildren_ = TaggedPointer(std::move(hashesAndChildren_), isBranch_, toAllocate); } -std::optional -SHAMapInnerNode::getChildIndex(int i) const +std::optional +SHAMapInnerNode::getChildIndex(unsigned int i) const { return hashesAndChildren_.getChildIndex(isBranch_, i); } @@ -89,7 +89,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const if (thisIsSparse) { - int cloneChildIndex = 0; + auto cloneChildIndex = 0u; iterNonEmptyChildIndexes([&](auto branchNum, auto indexNum) { cloneHashes[cloneChildIndex++] = thisHashes[indexNum]; }); @@ -105,7 +105,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const if (thisIsSparse) { - int cloneChildIndex = 0; + auto cloneChildIndex = 0u; iterNonEmptyChildIndexes([&](auto branchNum, auto indexNum) { cloneChildren[cloneChildIndex++] = thisChildren[indexNum]; }); @@ -133,12 +133,12 @@ SHAMapInnerNode::makeFullInner(Slice data, SHAMapHash const& hash, bool hashVali auto hashes = ret->hashesAndChildren_.getHashes(); - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { hashes[i].asUInt256() = si.getBitString<256>(); if (hashes[i].isNonZero()) - ret->isBranch_ |= (1 << i); + ret->isBranch_ |= (1u << i); } ret->resizeChildArrays(ret->getBranchCount()); @@ -182,7 +182,7 @@ SHAMapInnerNode::makeCompressedInner(Slice data) hashes[pos].asUInt256() = hash; if (hashes[pos].isNonZero()) - ret->isBranch_ |= (1 << pos); + ret->isBranch_ |= (1u << pos); } ret->resizeChildArrays(ret->getBranchCount()); @@ -267,20 +267,19 @@ SHAMapInnerNode::getString(SHAMapNodeID const& id) const // We are modifying an inner node void -SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) +SHAMapInnerNode::setChild(unsigned int branch, SHAMapTreeNodePtr child) { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), "xrpl::SHAMapInnerNode::setChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::setChild : valid branch input"); XRPL_ASSERT(cowid_, "xrpl::SHAMapInnerNode::setChild : nonzero cowid"); XRPL_ASSERT(child.get() != this, "xrpl::SHAMapInnerNode::setChild : valid child input"); auto const dstIsBranch = [&] { if (child) { - return isBranch_ | (1u << m); + return isBranch_ | (1u << branch); } - return isBranch_ & ~(1u << m); + return isBranch_ & ~(1u << branch); }(); auto const dstToAllocate = popcnt16(dstIsBranch); @@ -293,8 +292,8 @@ SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) if (child) { - auto const childIndex = - *getChildIndex(m); // NOLINT(bugprone-unchecked-optional-access) isBranch_ set above + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) isBranch_ set above + auto const childIndex = *getChildIndex(branch); auto [_, hashes, children] = hashesAndChildren_.getHashesAndChildren(); hashes[childIndex].zero(); children[childIndex] = std::move(child); @@ -309,25 +308,24 @@ SHAMapInnerNode::setChild(int m, SHAMapTreeNodePtr child) // finished modifying, now make shareable void -SHAMapInnerNode::shareChild(int m, SHAMapTreeNodePtr const& child) +SHAMapInnerNode::shareChild(unsigned int branch, SHAMapTreeNodePtr const& child) { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), "xrpl::SHAMapInnerNode::shareChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::shareChild : valid branch input"); XRPL_ASSERT(cowid_, "xrpl::SHAMapInnerNode::shareChild : nonzero cowid"); XRPL_ASSERT(child, "xrpl::SHAMapInnerNode::shareChild : non-null child input"); XRPL_ASSERT(child.get() != this, "xrpl::SHAMapInnerNode::shareChild : valid child input"); - XRPL_ASSERT(!isEmptyBranch(m), "xrpl::SHAMapInnerNode::shareChild : non-empty branch input"); + XRPL_ASSERT( + !isEmptyBranch(branch), "xrpl::SHAMapInnerNode::shareChild : non-empty branch input"); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) assert above - hashesAndChildren_.getChildren()[*getChildIndex(m)] = child; + hashesAndChildren_.getChildren()[*getChildIndex(branch)] = child; } SHAMapTreeNode* -SHAMapInnerNode::getChildPointer(int branch) +SHAMapInnerNode::getChildPointer(unsigned int branch) { XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::getChildPointer : valid branch input"); + branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChildPointer : valid branch input"); XRPL_ASSERT( !isEmptyBranch(branch), "xrpl::SHAMapInnerNode::getChildPointer : non-empty branch input"); @@ -340,11 +338,9 @@ SHAMapInnerNode::getChildPointer(int branch) } SHAMapTreeNodePtr -SHAMapInnerNode::getChild(int branch) +SHAMapInnerNode::getChild(unsigned int branch) { - XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::getChild : valid branch input"); + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChild : valid branch input"); XRPL_ASSERT(!isEmptyBranch(branch), "xrpl::SHAMapInnerNode::getChild : non-empty branch input"); auto const index = @@ -356,23 +352,20 @@ SHAMapInnerNode::getChild(int branch) } SHAMapHash const& -SHAMapInnerNode::getChildHash(int m) const +SHAMapInnerNode::getChildHash(unsigned int branch) const { - XRPL_ASSERT( - (m >= 0) && (m < kBranchFactor), - "xrpl::SHAMapInnerNode::getChildHash : valid branch input"); - if (auto const i = getChildIndex(m)) + XRPL_ASSERT(branch < kBranchFactor, "xrpl::SHAMapInnerNode::getChildHash : valid branch input"); + if (auto const i = getChildIndex(branch)) return hashesAndChildren_.getHashes()[*i]; return kZeroShaMapHash; } SHAMapTreeNodePtr -SHAMapInnerNode::canonicalizeChild(int branch, SHAMapTreeNodePtr node) +SHAMapInnerNode::canonicalizeChild(unsigned int branch, SHAMapTreeNodePtr node) { XRPL_ASSERT( - branch >= 0 && branch < kBranchFactor, - "xrpl::SHAMapInnerNode::canonicalizeChild : valid branch input"); + branch < kBranchFactor, "xrpl::SHAMapInnerNode::canonicalizeChild : valid branch input"); XRPL_ASSERT(node != nullptr, "xrpl::SHAMapInnerNode::canonicalizeChild : valid node input"); XRPL_ASSERT( !isEmptyBranch(branch), @@ -410,7 +403,7 @@ SHAMapInnerNode::invariants(bool isRoot) const if (numAllocated != kBranchFactor) { auto const branchCount = getBranchCount(); - for (int i = 0; i < branchCount; ++i) + for (auto i = 0u; i < branchCount; ++i) { XRPL_ASSERT( hashes[i].isNonZero(), @@ -422,12 +415,12 @@ SHAMapInnerNode::invariants(bool isRoot) const } else { - for (int i = 0; i < kBranchFactor; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (hashes[i].isNonZero()) { XRPL_ASSERT( - (isBranch_ & (1 << i)), + (isBranch_ & (1u << i)), "xrpl::SHAMapInnerNode::invariants : valid branch when " "nonzero hash"); if (children[i] != nullptr) @@ -437,7 +430,7 @@ SHAMapInnerNode::invariants(bool isRoot) const else { XRPL_ASSERT( - (isBranch_ & (1 << i)) == 0, + (isBranch_ & (1u << i)) == 0u, "xrpl::SHAMapInnerNode::invariants : valid branch when " "zero hash"); } diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index a511fc038c..ecde22a63d 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -16,7 +16,7 @@ namespace xrpl { static uint256 const& depthMask(unsigned int depth) { - static constexpr auto kMaskSize = 65; + static constexpr auto kMaskSize = SHAMap::kLeafDepth + 1; struct MasksT { @@ -25,7 +25,7 @@ depthMask(unsigned int depth) MasksT() { uint256 selector; - for (int i = 0; i < kMaskSize - 1; i += 2) + for (auto i = 0u; i < kMaskSize - 1; i += 2) { entry[i] = selector; *(selector.begin() + (i / 2)) = 0xF0; @@ -60,10 +60,10 @@ SHAMapNodeID::getRawString() const } SHAMapNodeID -SHAMapNodeID::getChildNodeID(unsigned int m) const +SHAMapNodeID::getChildNodeID(unsigned int branch) const { XRPL_ASSERT( - m < SHAMap::kBranchFactor, "xrpl::SHAMapNodeID::getChildNodeID : valid branch input"); + branch < SHAMap::kBranchFactor, "xrpl::SHAMapNodeID::getChildNodeID : valid branch input"); // A SHAMap has exactly 65 levels, so nodes must not exceed that // depth; if they do, this breaks the invariant of never allowing @@ -83,7 +83,7 @@ SHAMapNodeID::getChildNodeID(unsigned int m) const Throw("Incorrect mask for " + to_string(*this)); SHAMapNodeID node{depth_ + 1, id_}; - node.id_.begin()[depth_ / 2] |= ((depth_ & 1) != 0u) ? m : (m << 4); + node.id_.begin()[depth_ / 2] |= ((depth_ & 1) != 0u) ? branch : (branch << 4); return node; } @@ -127,10 +127,9 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash) } SHAMapNodeID -SHAMapNodeID::createID(int depth, uint256 const& key) +SHAMapNodeID::createID(unsigned int depth, uint256 const& key) { - XRPL_ASSERT( - depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); + XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); return SHAMapNodeID(depth, key & depthMask(depth)); } diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index cbed6885c9..e6948ec3ac 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -54,15 +54,15 @@ SHAMap::visitNodes(std::function const& function) const if (!root_->isInner()) return; - using StackEntry = std::pair>; + using StackEntry = std::pair>; std::stack> stack; auto node = intr_ptr::staticPointerCast(root_); - int pos = 0; + auto pos = 0u; while (true) { - while (pos < 16) + while (pos < kBranchFactor) { if (!node->isEmptyBranch(pos)) { @@ -77,10 +77,10 @@ SHAMap::visitNodes(std::function const& function) const else { // If there are no more children, don't push this node - while ((pos != 15) && (node->isEmptyBranch(pos + 1))) + while ((pos != kBranchFactor - 1u) && (node->isEmptyBranch(pos + 1))) ++pos; - if (pos != 15) + if (pos != kBranchFactor - 1u) { // save next position to resume at stack.emplace(pos + 1, std::move(node)); @@ -144,7 +144,7 @@ SHAMap::visitDifferences( return; // 2) push non-matching child inner nodes - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!node->isEmptyBranch(i)) { @@ -176,13 +176,13 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) { SHAMapInnerNode*& node = std::get<0>(se); SHAMapNodeID& nodeID = std::get<1>(se); - int& firstChild = std::get<2>(se); - int& currentChild = std::get<3>(se); + auto& firstChild = std::get<2>(se); + auto& currentChild = std::get<3>(se); bool& fullBelow = std::get<4>(se); - while (currentChild < 16) + while (currentChild < kBranchFactor) { - int const branch = (firstChild + currentChild++) % 16; + auto const branch = (firstChild + currentChild++) % kBranchFactor; if (node->isEmptyBranch(branch)) continue; @@ -262,7 +262,7 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn) int complete = 0; while (complete != mn.deferred) { - std::tuple deferredNode; + MissingNodes::DeferredNode deferredNode; { std::unique_lock lock{mn.deferLock}; @@ -423,7 +423,7 @@ SHAMap::getNodeFat( while ((node != nullptr) && node->isInner() && (nodeID.getDepth() < wanted.getDepth())) { - int const branch = selectBranch(nodeID, wanted.getNodeID()); + auto const branch = selectBranch(nodeID, wanted.getNodeID()); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; @@ -444,7 +444,7 @@ SHAMap::getNodeFat( return false; } - std::stack> stack; + std::stack> stack; stack.emplace(node, nodeID, depth); Serializer s(8192); @@ -464,12 +464,12 @@ SHAMap::getNodeFat( // We descend inner nodes with only a single child // without decrementing the depth auto inner = safeDowncast(node); - int const bc = inner->getBranchCount(); + auto const bc = inner->getBranchCount(); if ((depth > 0) || (bc == 1)) { // We need to process this node's children - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (!inner->isEmptyBranch(i)) { @@ -575,8 +575,7 @@ SHAMap::addKnownNode( !safeDowncast(currNode)->isFullBelow(generation) && (currNodeID.getDepth() < nodeID.getDepth())) { - int const branch = selectBranch(currNodeID, nodeID.getNodeID()); - XRPL_ASSERT(branch >= 0, "xrpl::SHAMap::addKnownNode : valid branch"); + auto const branch = selectBranch(currNodeID, nodeID.getNodeID()); auto inner = safeDowncast(currNode); if (inner->isEmptyBranch(branch)) { @@ -686,7 +685,7 @@ SHAMap::deepCompare(SHAMap& other) const return false; auto nodeInner = safeDowncast(node); auto otherInner = safeDowncast(otherNode); - for (int i = 0; i < 16; ++i) + for (auto i = 0u; i < kBranchFactor; ++i) { if (nodeInner->isEmptyBranch(i)) { @@ -725,7 +724,7 @@ SHAMap::hasInnerNode(SHAMapNodeID const& targetNodeID, SHAMapHash const& targetN while (node->isInner() && (nodeID.getDepth() < targetNodeID.getDepth())) { - int const branch = selectBranch(nodeID, targetNodeID.getNodeID()); + auto const branch = selectBranch(nodeID, targetNodeID.getNodeID()); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; @@ -751,7 +750,20 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const do { - int const branch = selectBranch(nodeID, tag); + // An inner node is only reachable here at a depth below kLeafDepth in a well-formed map, + // where the loop always finds a leaf first. A malformed map could still have an inner + // node claiming kLeafDepth, and getChildNodeID below throws in that case: reject rather + // than let the throw escape uncaught. Not reachable through any public entry point, + // since addKnownNode already marks such a map invalid, so no test can cover this. + if (nodeID.getDepth() >= kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::hasLeafNode : inner node at leaf depth"); + return false; + // LCOV_EXCL_STOP + } + + auto const branch = selectBranch(nodeID, tag); auto inner = safeDowncast(node); if (inner->isEmptyBranch(branch)) return false; // Dead end, node must not be here @@ -803,7 +815,7 @@ SHAMap::getProofPath(uint256 const& key) const bool SHAMap::verifyProofPath(uint256 const& rootHash, uint256 const& key, std::vector const& path) { - if (path.empty() || path.size() > 65) + if (path.empty() || path.size() > kLeafDepth + 1u) return false; SHAMapHash hash{rootHash}; @@ -819,10 +831,10 @@ SHAMap::verifyProofPath(uint256 const& rootHash, uint256 const& key, std::vector if (node->getHash() != hash) return false; - auto depth = std::distance(path.rbegin(), rit); + auto const depth = std::distance(path.rbegin(), rit); if (node->isInner()) { - auto nodeId = SHAMapNodeID::createID(depth, key); + auto nodeId = SHAMapNodeID::createID(static_cast(depth), key); hash = safeDowncast(node.get()) ->getChildHash(selectBranch(nodeId, key)); } diff --git a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp index 531dba59f9..abd669d446 100644 --- a/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp +++ b/src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp @@ -75,7 +75,7 @@ getSHAMapNodeID(protocol::TMLedgerNode const& ledgerNode, SHAMapTreeNode const& if (treeNode.isLeaf()) { auto const key = leafKey(treeNode); - auto const expectedID = SHAMapNodeID::createID(static_cast(nodeID->getDepth()), key); + auto const expectedID = SHAMapNodeID::createID(nodeID->getDepth(), key); SOMETIMES( nodeID->getNodeID() != expectedID.getNodeID(), "xrpl::getSHAMapNodeID : legacy leaf ID inconsistent with key"); From c49789086ad3b031cd527fa7ed2e687e81fdfd4a Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 12:52:20 +0000 Subject: [PATCH 2/5] fix: Extend locked-MPToken unauthorize check to fixCleanup3_4_0 (#8004) --- .../tx/transactors/token/MPTokenAuthorize.cpp | 27 ++++----- src/test/app/MPToken_test.cpp | 58 ++++++++++++++++++- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp index 0aeb6f33d1..c19b8f64d7 100644 --- a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp +++ b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp @@ -37,6 +37,7 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) { auto const accountID = ctx.tx[sfAccount]; auto const holderID = ctx.tx[~sfHolder]; + auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); // if non-issuer account submits this tx, then they are trying either: // 1. Unauthorize/delete MPToken @@ -51,9 +52,8 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) // There is an edge case where all holders have zero balance, issuance // is legally destroyed, then outstanding MPT(s) are deleted afterwards. - // Thus, there is no need to check for the existence of the issuance if - // the MPT is being deleted with a zero balance. Check for unauthorize - // before fetching the MPTIssuance object. + // Thus, the unauthorize/delete path below does not require the issuance + // to exist when the MPT is being deleted with a zero balance. // if holder wants to delete/unauthorize a mpt if (ctx.tx.isFlag(tfMPTUnauthorize)) @@ -63,8 +63,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[sfMPTAmount] != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE @@ -73,21 +71,24 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[~sfLockedAmount].value_or(0) != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE return tecHAS_OBLIGATIONS; } - if (ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + if (ctx.view.rules().enabled(fixCleanup3_4_0)) + { + if (sleMptIssuance && sleMpt->isFlag(lsfMPTLocked)) + return tecNO_PERMISSION; + } + else if ( + ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + { return tecNO_PERMISSION; + } if (ctx.view.rules().enabled(featureConfidentialTransfer)) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - // if there still existing encrypted balances of MPT in // circulation if (sleMptIssuance && @@ -106,9 +107,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) } // Now test when the holder wants to hold/create/authorize a new MPT - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; @@ -126,7 +124,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if (!sleHolder) return tecNO_DST; - auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index b392dca758..7086adf743 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -789,7 +789,7 @@ class MPToken_test : public beast::unit_test::Suite // locks up bob's mptoken again mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); - if (!features[featureSingleAssetVault]) + if (!features[featureSingleAssetVault] && !features[fixCleanup3_4_0]) { // Delete bob's mptoken even though it is locked mptAlice.authorize({.account = bob, .flags = tfMPTUnauthorize}); @@ -7657,6 +7657,56 @@ class MPToken_test : public beast::unit_test::Suite 0, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION); } + void + testLockedMPTokenDestroyedIssuance(FeatureBitset features) + { + testcase("Locked MPToken with destroyed issuance"); + + using namespace test::jtx; + Account const alice("alice"); // issuer + Account const bob("bob"); // holder + + Env env{*this, features}; + env.fund(XRP(1'000), alice, bob); + env.close(); + MPTTester mptAlice( + {.env = env, .issuer = alice, .holders = {bob}, .flags = kMptDexFlags | tfMPTCanLock}); + + // alice locks bob's mptoken individually + mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); + + // alice destroys her issuance. This succeeds: MPTokenIssuanceDestroy + // only requires that the issuance has no outstanding balance; it does + // not require that all holder MPTokens have been deleted first. + mptAlice.destroy({.ownerCount = 0}); + + if (!features[featureSingleAssetVault] || features[fixCleanup3_4_0]) + { + // pre SAV or post Cleanup340 amendment: bob deletes the dangling locked MPToken + mptAlice.authorize({.account = bob, .holderCount = 0, .flags = tfMPTUnauthorize}); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + else + { + // bob cannot delete his locked MPToken, even though the issuance + // no longer exists. + mptAlice.authorize( + {.account = bob, .flags = tfMPTUnauthorize, .err = tecNO_PERMISSION}); + + // and the lock can never be cleared, because unlocking + // requires the (destroyed) issuance + mptAlice.set( + {.account = alice, + .holder = bob, + .flags = tfMPTUnlock, + .err = tecOBJECT_NOT_FOUND}); + + // the dangling locked MPToken survives + BEAST_EXPECT(env.current()->exists(keylet::mptoken(mptAlice.issuanceID(), bob.id()))); + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + } + public: void run() override @@ -7703,7 +7753,9 @@ public: testSetValidation(all - featurePermissionedDomains); testSetValidation(all); + testSetEnabled(all - featureSingleAssetVault - fixCleanup3_4_0); testSetEnabled(all - featureSingleAssetVault); + testSetEnabled(all - fixCleanup3_4_0); testSetEnabled(all); // MPT clawback @@ -7770,6 +7822,10 @@ public: // Fixes testFixDoubleOwnerCount(all); + testLockedMPTokenDestroyedIssuance(all); + testLockedMPTokenDestroyedIssuance(all - fixCleanup3_4_0); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault - fixCleanup3_4_0); } }; From ca6121c5b34520f304796fdc1a57c2bac5806a83 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 20:58:46 +0000 Subject: [PATCH 3/5] feat: Enforce MPT CanTransfer on AMM LPTokens transfers (#7418) --- include/xrpl/ledger/View.h | 20 +++ src/libxrpl/ledger/View.cpp | 27 ++++ src/libxrpl/ledger/helpers/TokenHelpers.cpp | 9 ++ src/libxrpl/tx/paths/DirectStep.cpp | 11 +- src/test/app/LPTokenTransfer_test.cpp | 135 ++++++++++++++++++++ 5 files changed, 200 insertions(+), 2 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index e8b4a932d0..0893612bac 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -85,6 +85,26 @@ isLPTokenFrozen( Asset const& asset, Asset const& asset2); +/** + * Check whether an AMM LPToken may be transferred between @p from and @p to. + * + * @p lpTokenIssuer is the issuer of the LPToken being moved. If it is not an + * AMM account the token is not an LPToken and the transfer is unconditionally + * permitted. Otherwise, for each MPT pool asset of that AMM, canTransfer() must + * permit the transfer (which exempts the MPT issuer). Non-MPT pool assets are + * always transferable by this check, so it is implicitly gated by + * featureMPTokensV2 (MPTs can only be AMM pool assets once V2 is enabled). + * + * @return tesSUCCESS if permitted, otherwise the canTransfer() failure code + * (e.g. tecNO_AUTH) of the first MPT pool asset that disallows it. + */ +[[nodiscard]] TER +canTransferLPToken( + ReadView const& view, + AccountID const& from, + AccountID const& to, + AccountID const& lpTokenIssuer); + // Return the list of enabled amendments [[nodiscard]] std::set getEnabledAmendments(ReadView const& view); diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 2dd70e2950..0544771973 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -138,6 +138,33 @@ isLPTokenFrozen( return isFrozen(view, account, asset) || isFrozen(view, account, asset2); } +TER +canTransferLPToken( + ReadView const& view, + AccountID const& from, + AccountID const& to, + AccountID const& lpTokenIssuer) +{ + // Only AMM-issued LPTokens are subject to this check. The LPToken's issuer + // is the AMM account; if it is not an AMM, this is not an LPToken. + auto const sleIssuer = view.read(keylet::account(lpTokenIssuer)); + if (!sleIssuer || !sleIssuer->isFieldPresent(sfAMMID)) + return tesSUCCESS; + + auto const sleAmm = view.read(keylet::amm((*sleIssuer)[sfAMMID])); + if (!sleAmm) + return tecINTERNAL; // LCOV_EXCL_LINE + + auto const transferable = [&](Asset const& a) -> TER { + if (!a.holds()) + return tesSUCCESS; + return canTransfer(view, a.get(), from, to); + }; + if (auto const err = transferable((*sleAmm)[sfAsset]); !isTesSuccess(err)) + return err; + return transferable((*sleAmm)[sfAsset2]); +} + bool areCompatible( ReadView const& validLedger, diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 79e10cdf79..9e3452ccae 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -309,6 +309,15 @@ getLineIfUsable( } } } + + // An LPToken whose AMM pool contains an MPT that forbids transfers is not + // spendable. Issuer is the LPToken's AMM account; canTransferLPToken is + // a no-op for non-AMM issuers and non-MPT pool assets, so this is implicitly + // gated by featureMPTokensV2. + if (!isTesSuccess(canTransferLPToken(view, account, account, issuer))) + { + return nullptr; + } } return sle; diff --git a/src/libxrpl/tx/paths/DirectStep.cpp b/src/libxrpl/tx/paths/DirectStep.cpp index f8f12bd421..1854bd3632 100644 --- a/src/libxrpl/tx/paths/DirectStep.cpp +++ b/src/libxrpl/tx/paths/DirectStep.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -845,8 +846,14 @@ DirectStepI::check(StrandContext const& ctx) const // pure issue/redeem can't be frozen if (!(ctx.isLast && ctx.isFirst)) { - auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); - if (!isTesSuccess(ter)) + if (auto const ter = checkFreeze(ctx.view, src_, dst_, currency_); !isTesSuccess(ter)) + return ter; + + // An LPToken redeemed against its AMM (dst_ is the LPToken issuer on + // this hop) cannot move if a pool asset is an MPT that forbids + // transfers between these accounts. A no-op unless dst_ is an AMM whose + // pool holds such an MPT (so it is implicitly gated by featureMPTokensV2). + if (auto const ter = canTransferLPToken(ctx.view, src_, dst_, dst_); !isTesSuccess(ter)) return ter; } diff --git a/src/test/app/LPTokenTransfer_test.cpp b/src/test/app/LPTokenTransfer_test.cpp index e30e37ed98..3e72094eb3 100644 --- a/src/test/app/LPTokenTransfer_test.cpp +++ b/src/test/app/LPTokenTransfer_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include // IWYU pragma: keep #include @@ -21,6 +22,8 @@ #include #include +#include + namespace xrpl::test { class LPTokenTransfer_test : public jtx::AMMTest @@ -433,6 +436,136 @@ class LPTokenTransfer_test : public jtx::AMMTest } } + void + testMPTCanTransferDirectStep(FeatureBitset features) + { + testcase("MPT CanTransfer DirectStep"); + + using namespace jtx; + + // An MPT can only be an AMM pool asset once featureMPTokensV2 is + // enabled, so this behavior is only meaningful when V2 is present, and + // is independent of fixFrozenLPTokenTransfer. + if (!features[featureMPTokensV2]) + return; + + // gw issues an MPT used as one of the AMM pool assets. gw (the MPT + // issuer) seeds the pool and hands LP tokens to alice. Transferring LP + // tokens between two non-issuer holders is only permitted when the + // pool MPT allows transfers (lsfMPTCanTransfer); issuer-involving + // transfers are always permitted. The check fires on the redeem step + // against the AMM account via canTransferLPToken(). + auto testLPTokenTransfer = [&](std::uint32_t mptFlags, bool poolXrpToBtc) { + Env env{*this, features}; + env.fund(XRP(30'000), gw_, alice_, bob_); + env.close(); + + // gw is the MPT issuer, so it may seed the pool regardless of + // whether the MPT permits third-party transfers. + MPT const btc = MPTTester( + {.env = env, .issuer = gw_, .holders = {alice_}, .pay = 1'000, .flags = mptFlags}); + + auto const asset1 = poolXrpToBtc ? XRP(10'000) : btc(10'000); + auto const asset2 = poolXrpToBtc ? btc(10'000) : XRP(10'000); + AMM const amm(env, gw_, asset1, asset2); + auto const lpIssue = amm.lptIssue(); + + env.trust(STAmount{lpIssue, 100'000}, alice_); + env.trust(STAmount{lpIssue, 100'000}, bob_); + env.close(); + + // Issuer-involving LP token transfer is always allowed (gw is the + // pool MPT's issuer), even when the MPT lacks CanTransfer. + env(pay(gw_, alice_, STAmount{lpIssue, 1'000})); + env.close(); + + // Transfer between two non-issuer holders is allowed only if the + // pool MPT has CanTransfer set; otherwise the redeem step against + // the AMM account blocks it with tecNO_AUTH. + if ((mptFlags & tfMPTCanTransfer) != 0u) + { + env(pay(alice_, bob_, STAmount{lpIssue, 100})); + } + else + { + env(pay(alice_, bob_, STAmount{lpIssue, 100}), Ter(tecNO_AUTH)); + } + env.close(); + }; + + // Pool MPT without CanTransfer blocks third-party LP token transfers. + testLPTokenTransfer(tfMPTCanTrade, true); + testLPTokenTransfer(tfMPTCanTrade, false); + + // Pool MPT with CanTransfer allows them. + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, true); + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, false); + } + + void + testMPTCanTransferOffer(FeatureBitset features) + { + testcase("MPT CanTransfer Offer"); + + using namespace jtx; + + if (!features[featureMPTokensV2]) + return; + + // Parity with frozen LP tokens for the order book: a non-transferable + // pool MPT makes the LP token un-spendable (canTransferLPToken zeroes + // the spendable balance in accountHolds, just as isLPTokenFrozen does), + // so an offer to sell it cannot be funded - the same tecUNFUNDED_OFFER + // outcome as freezing a pool asset (see testOfferCreation). + auto testLPTokenTransfer = [&](std::uint32_t mptFlags, bool poolXrpToBtc) { + Env env{*this, features}; + env.fund(XRP(30'000), gw_, carol_); + env.close(); + + MPT const btc = MPTTester( + {.env = env, .issuer = gw_, .holders = {carol_}, .pay = 1'000, .flags = mptFlags}); + + auto const asset1 = poolXrpToBtc ? XRP(10'000) : btc(10'000); + auto const asset2 = poolXrpToBtc ? btc(10'000) : XRP(10'000); + AMM const amm(env, gw_, asset1, asset2); + auto const lpIssue = amm.lptIssue(); + + env.trust(STAmount{lpIssue, 100'000}, carol_); + env.close(); + + // gw (the pool MPT issuer) seeds carol_ with LP tokens; issuer + // involving transfers are always allowed. + env(pay(gw_, carol_, STAmount{lpIssue, 1'000})); + env.close(); + + // carol_ tries to create an offer to sell the LP token. + if ((mptFlags & tfMPTCanTransfer) != 0u) + { + env(offer(carol_, XRP(10), STAmount{lpIssue, 10}), Txflags(tfPassive)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol_, 1)); + } + else + { + // Non-transferable pool MPT => LP token un-spendable => the + // sell offer is unfunded, just as if a pool asset were frozen. + env(offer(carol_, XRP(10), STAmount{lpIssue, 10}), + Txflags(tfPassive), + Ter(tecUNFUNDED_OFFER)); + env.close(); + BEAST_EXPECT(expectOffers(env, carol_, 0)); + } + }; + + // Pool MPT without CanTransfer: LP token sell offer is unfunded. + testLPTokenTransfer(tfMPTCanTrade, true); + testLPTokenTransfer(tfMPTCanTrade, false); + + // Pool MPT with CanTransfer: LP token sell offer is created. + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, true); + testLPTokenTransfer(tfMPTCanTrade | tfMPTCanTransfer, false); + } + public: void run() override @@ -447,6 +580,8 @@ public: testOfferCrossing(features); testCheck(features); testNFTOffers(features); + testMPTCanTransferDirectStep(features); + testMPTCanTransferOffer(features); } } }; From 1b226c8b2eb3d08b7018738adb1cddc6f6768372 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 17 Aug 2026 21:15:16 +0000 Subject: [PATCH 4/5] perf: Optimize MPT freeze checks to reduce redundant state reads (#7411) Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- include/xrpl/ledger/View.h | 7 ++ include/xrpl/ledger/helpers/MPTokenHelpers.h | 35 ++++++++++ src/libxrpl/ledger/View.cpp | 69 +++++++++++++++---- src/libxrpl/ledger/helpers/AMMHelpers.cpp | 3 +- src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | 64 +++++++++++++++-- src/libxrpl/ledger/helpers/TokenHelpers.cpp | 2 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 2 +- .../tx/transactors/escrow/EscrowCreate.cpp | 4 +- .../tx/transactors/escrow/EscrowFinish.cpp | 2 +- src/test/app/AMMMPT_test.cpp | 54 +++++++++++++++ 10 files changed, 216 insertions(+), 26 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 0893612bac..f7fd5b5a8c 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -78,6 +78,13 @@ isVaultPseudoAccountFrozen( MPTIssue const& mptShare, std::uint8_t depth); +[[nodiscard]] bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth); + [[nodiscard]] bool isLPTokenFrozen( ReadView const& view, diff --git a/include/xrpl/ledger/helpers/MPTokenHelpers.h b/include/xrpl/ledger/helpers/MPTokenHelpers.h index 7babefd196..6d26cf3cbc 100644 --- a/include/xrpl/ledger/helpers/MPTokenHelpers.h +++ b/include/xrpl/ledger/helpers/MPTokenHelpers.h @@ -29,6 +29,9 @@ namespace xrpl { [[nodiscard]] bool isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue); +[[nodiscard]] bool +isGlobalFrozen(SLE const& issuanceSle); + /** * Returns true if @p account's MPToken for @p mptIssue carries the * individual-lock flag (lsfMPTLocked). @@ -40,9 +43,29 @@ isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue); * receive tokens — it combines isIndividualFrozen, isGlobalFrozen, and * isVaultPseudoAccountFrozen into a single complete check. */ + [[nodiscard]] bool isIndividualFrozen(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue); +[[nodiscard]] bool +isIndividualFrozen(SLE const& mptSle); + +/** + * Returns true if @p account cannot send or receive tokens of @p mptIssue + * because a freeze applies. This is the complete check callers should use + * before moving MPT value: it combines @ref isGlobalFrozen (issuance-level + * lock), @ref isIndividualFrozen (per-holder lock bit), and the transitive + * vault pseudo-account check (if @p mptIssue is a vault share, the underlying + * asset is checked, and so on recursively up to @c maxAssetCheckDepth). + * + * The @c SLE overload takes an already-loaded ltMPTOKEN or ltMPTOKEN_ISSUANCE + * ledger entry; for ltMPTOKEN it can skip the per-holder individual-lock lookup. + * @ref isAnyFrozen answers the same question for a set of accounts and returns true + * if the freeze applies to any of them. + * + * @param depth Current recursion depth for the vault-share walk. Callers + * outside this module should leave it at the default. + */ [[nodiscard]] bool isFrozen( ReadView const& view, @@ -50,6 +73,18 @@ isFrozen( MPTIssue const& mptIssue, std::uint8_t depth = 0); +/** + * SLE overload: pass an already-loaded ltMPTOKEN (holder row) or + * ltMPTOKEN_ISSUANCE to reuse it for the freeze checks and avoid re-reading + * the same object. For an ltMPTOKEN, @p sle is used directly for the + * individual-lock check and the issuance is read once for global-freeze and + * vault-pseudo-account. For an ltMPTOKEN_ISSUANCE, @p sle is used directly + * for global-freeze and vault-pseudo-account, and the caller's holder row is + * read for the individual-lock check. + */ +[[nodiscard]] bool +isFrozen(ReadView const& view, AccountID const& account, SLE const& sle, std::uint8_t depth = 0); + [[nodiscard]] bool isAnyFrozen( ReadView const& view, diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0544771973..e01ae2e492 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -61,12 +61,10 @@ hasExpired( : view.parentCloseTime() > boundary; } -bool -isVaultPseudoAccountFrozen( - ReadView const& view, - AccountID const& account, - MPTIssue const& mptShare, - std::uint8_t depth) +namespace { + +std::optional +checkVaultPseudoAccountFrozenPreconditions(ReadView const& view, std::uint8_t depth) { if (!view.rules().enabled(featureSingleAssetVault)) return false; @@ -74,26 +72,37 @@ isVaultPseudoAccountFrozen( if (depth >= kMaxAssetCheckDepth) { // LCOV_EXCL_START - UNREACHABLE("xrpl::View::isVaultPseudoAccountFrozen : reached asset check depth"); + UNREACHABLE( + "xrpl::View::checkVaultPseudoAccountFrozenPreconditions : reached asset check depth"); return true; // LCOV_EXCL_STOP } - auto const mptIssuance = view.read(keylet::mptokenIssuance(mptShare.getMptID())); - if (mptIssuance == nullptr) - return false; // zero MPToken won't block deletion of MPTokenIssuance + return std::nullopt; +} - auto const issuer = mptIssuance->getAccountID(sfIssuer); +bool +isVaultPseudoAccountFrozenForIssuance( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth) +{ + XRPL_ASSERT( + issuanceSle.getType() == ltMPTOKEN_ISSUANCE, + "xrpl::isVaultPseudoAccountFrozenForIssuance : MPTokenIssuance SLE"); + + auto const issuer = issuanceSle.getAccountID(sfIssuer); // Post-fixCleanup3_2_0: vault shares carry sfReferenceHolding pointing // to the vault pseudo's MPToken or RippleState for the underlying. // Read it to derive the underlying asset and recurse, skipping the // issuer-account-then-vault chain. Pre-amendment shares (no field) // fall back to the chain lookup below. - if (mptIssuance->isFieldPresent(sfReferenceHolding)) + if (issuanceSle.isFieldPresent(sfReferenceHolding)) { auto const sleHolding = - view.read(keylet::unchecked(mptIssuance->getFieldH256(sfReferenceHolding))); + view.read(keylet::unchecked(issuanceSle.getFieldH256(sfReferenceHolding))); if (!sleHolding) { // LCOV_EXCL_START @@ -102,7 +111,7 @@ isVaultPseudoAccountFrozen( // LCOV_EXCL_STOP } return isAnyFrozen( - view, {issuer, account}, assetOfHolding(*mptIssuance, *sleHolding), depth + 1); + view, {issuer, account}, assetOfHolding(issuanceSle, *sleHolding), depth + 1); } auto const mptIssuer = view.read(keylet::account(issuer)); @@ -128,6 +137,38 @@ isVaultPseudoAccountFrozen( return isAnyFrozen(view, {issuer, account}, vault->at(sfAsset), depth + 1); } +} // namespace + +bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + SLE const& issuanceSle, + std::uint8_t depth) +{ + if (auto const result = checkVaultPseudoAccountFrozenPreconditions(view, depth)) + return *result; + + return isVaultPseudoAccountFrozenForIssuance(view, account, issuanceSle, depth); +} + +bool +isVaultPseudoAccountFrozen( + ReadView const& view, + AccountID const& account, + MPTIssue const& mptShare, + std::uint8_t depth) +{ + if (auto const result = checkVaultPseudoAccountFrozenPreconditions(view, depth)) + return *result; + + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptShare.getMptID())); + if (issuanceSle == nullptr) + return false; // zero MPToken won't block deletion of MPTokenIssuance + + return isVaultPseudoAccountFrozenForIssuance(view, account, *issuanceSle, depth); +} + bool isLPTokenFrozen( ReadView const& view, diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index df6d335085..fcad22d2d5 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -633,7 +634,7 @@ ammAccountHolds(ReadView const& view, AccountID const& ammAccountID, Asset const return asset.visit( [&](MPTIssue const& issue) { if (auto const sle = view.read(keylet::mptoken(issue, ammAccountID)); - sle && !isFrozen(view, ammAccountID, issue)) + sle && !isFrozen(view, ammAccountID, *sle)) return STAmount{issue, (*sle)[sfMPTAmount]}; return STAmount{asset}; }, diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index b239d0d3d1..73d5fdb1d5 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -42,18 +42,35 @@ bool isGlobalFrozen(ReadView const& view, MPTIssue const& mptIssue) { if (auto const sle = view.read(keylet::mptokenIssuance(mptIssue.getMptID()))) - return sle->isFlag(lsfMPTLocked); + return isGlobalFrozen(*sle); return false; } +bool +isGlobalFrozen(SLE const& issuanceSle) +{ + XRPL_ASSERT( + issuanceSle.getType() == ltMPTOKEN_ISSUANCE, "xrpl::isGlobalFrozen : MPTokenIssuance SLE"); + + return issuanceSle.isFlag(lsfMPTLocked); +} + bool isIndividualFrozen(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue) { if (auto const sle = view.read(keylet::mptoken(mptIssue.getMptID(), account))) - return sle->isFlag(lsfMPTLocked); + return isIndividualFrozen(*sle); return false; } +bool +isIndividualFrozen(SLE const& mptSle) +{ + XRPL_ASSERT(mptSle.getType() == ltMPTOKEN, "xrpl::isIndividualFrozen : MPToken SLE"); + + return mptSle.isFlag(lsfMPTLocked); +} + bool isFrozen( ReadView const& view, @@ -65,6 +82,34 @@ isFrozen( isVaultPseudoAccountFrozen(view, account, mptIssue, depth); } +bool +isFrozen(ReadView const& view, AccountID const& account, SLE const& sle, std::uint8_t depth) +{ + XRPL_ASSERT( + sle.getType() == ltMPTOKEN || sle.getType() == ltMPTOKEN_ISSUANCE, + "xrpl::isFrozen : MPToken or MPTokenIssuance SLE"); + + if (sle.getType() == ltMPTOKEN) + { + XRPL_ASSERT(sle[sfAccount] == account, "xrpl::isFrozen : valid MPToken holder"); + + MPTID const mptID = sle[sfMPTokenIssuanceID]; + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptID)); + + if ((issuanceSle && isGlobalFrozen(*issuanceSle)) || isIndividualFrozen(sle)) + return true; + + if (issuanceSle) + return isVaultPseudoAccountFrozen(view, account, *issuanceSle, depth); + + return isVaultPseudoAccountFrozen(view, account, MPTIssue{mptID}, depth); + } + + MPTIssue const mptIssue{sle[sfSequence], sle[sfIssuer]}; + return isGlobalFrozen(sle) || isIndividualFrozen(view, account, mptIssue) || + isVaultPseudoAccountFrozen(view, account, sle, depth); +} + [[nodiscard]] bool isAnyFrozen( ReadView const& view, @@ -72,7 +117,8 @@ isAnyFrozen( MPTIssue const& mptIssue, std::uint8_t depth) { - if (isGlobalFrozen(view, mptIssue)) + auto const issuanceSle = view.read(keylet::mptokenIssuance(mptIssue.getMptID())); + if (issuanceSle && isGlobalFrozen(*issuanceSle)) return true; for (auto const& account : accounts) @@ -81,9 +127,15 @@ isAnyFrozen( return true; } - return std::ranges::any_of(accounts, [&](auto const& account) { - return isVaultPseudoAccountFrozen(view, account, mptIssue, depth); - }); + // Pass the issuance SLE when we have it to avoid re-reading it per account; + // otherwise defer to the MPTIssue overload, which handles a missing issuance. + auto const anyVaultFrozen = [&](auto const& shareOrIssuance) { + return std::ranges::any_of(accounts, [&](auto const& account) { + return isVaultPseudoAccountFrozen(view, account, shareOrIssuance, depth); + }); + }; + + return issuanceSle ? anyVaultFrozen(*issuanceSle) : anyVaultFrozen(mptIssue); } Rate diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index 9e3452ccae..7ebfa64bcf 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -439,7 +439,7 @@ accountHolds( auto const sleMpt = view.read(keylet::mptoken(mptIssue.getMptID(), account)); if (!sleMpt || - (zeroIfFrozen == FreezeHandling::ZeroIfFrozen && isFrozen(view, account, mptIssue))) + (zeroIfFrozen == FreezeHandling::ZeroIfFrozen && isFrozen(view, account, *sleMpt))) { amount.clear(mptIssue); } diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 12ec078c82..d323718bd2 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -900,7 +900,7 @@ ValidMPTTransfer::finalize( // Check once: if any involved account is frozen, the whole issuance transfer is // considered frozen. Only need to check for frozen if there is a transfer of funds. if (!invalidTransfer && - (isFrozen(view, account, MPTIssue{mptID}) || + (isFrozen(view, account, *sleIssuance) || !isAuthorized(view, mptID, account, reqAuth))) { invalidTransfer = true; diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp index 212f9da075..0fe27fb3ba 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp @@ -304,11 +304,11 @@ escrowCreatePreclaimHelper( return ter; // If the issuer has frozen the account, return tecLOCKED - if (isFrozen(ctx.view, account, mptIssue)) + if (isFrozen(ctx.view, account, *sleIssuance)) return tecLOCKED; // If the issuer has frozen the destination, return tecLOCKED - if (isFrozen(ctx.view, dest, mptIssue)) + if (isFrozen(ctx.view, dest, *sleIssuance)) return tecLOCKED; // If the mpt cannot be transferred, return tecNO_AUTH diff --git a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp index 32f4d9ec48..aa352d5e98 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp @@ -186,7 +186,7 @@ escrowFinishPreclaimHelper( return ter; // If the issuer has frozen the destination, return tecLOCKED - if (isFrozen(ctx.view, dest, mptIssue)) + if (isFrozen(ctx.view, dest, *sleIssuance)) return tecLOCKED; return tesSUCCESS; diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 90a267f56f..bfd2d529b5 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -27,6 +27,8 @@ #include #include #include +#include +#include #include #include #include @@ -7421,6 +7423,57 @@ private: } } + void + testDanglingAMMMPTokenFreezeCheck() + { + testcase("Dangling AMM MPToken freeze check"); + + using namespace jtx; + FeatureBitset const all{testableAmendments()}; + + Env env(*this, all); + + env.fund(XRP(1'000), gw_, alice_); + MPTTester usd({.env = env, .issuer = gw_}); + MPTTester const btc({.env = env, .issuer = gw_}); + + AMM amm(env, gw_, usd(10'000), btc(10'000)); + for (auto i = 0; i < kMaxDeletableAmmTrustLines + 10; ++i) + { + Account const a{std::to_string(i)}; + env.fund(XRP(1'000), a); + env(trust(a, STAmount{amm.lptIssue(), 10'000})); + env.close(); + } + + // With too many LP-token trust lines to delete in one pass, the AMM + // remains in an empty state with zero-balance MPToken objects. + amm.withdrawAll(gw_); + BEAST_EXPECT(amm.ammExists()); + BEAST_EXPECT(amm.expectBalances(usd(0), btc(0), IOUAmount{0})); + + auto const ammToken = env.le(keylet::mptoken(usd.issuanceID(), amm.ammAccount())); + if (!BEAST_EXPECT(ammToken)) + return; + BEAST_EXPECT((*ammToken)[sfMPTAmount] == 0); + + usd.destroy(); + BEAST_EXPECT(env.le(keylet::mptokenIssuance(usd.issuanceID())) == nullptr); + BEAST_EXPECT(!isFrozen(*env.current(), amm.ammAccount(), *ammToken)); + // A Payment cannot cross this empty AMM because BookStep skips AMMs + // with zero LPTokenBalance. Probe the same ZeroIfFrozen balance read + // used by AMM accounting. + auto const balance = accountHolds( + *env.current(), + amm.ammAccount(), + MPTIssue{usd.issuanceID()}, + FreezeHandling::ZeroIfFrozen, + AuthHandling::IgnoreAuth, + env.journal); + + BEAST_EXPECT(balance == usd(0)); + } + void run() override { @@ -7461,6 +7514,7 @@ private: testDepositIntegralOverflowMPT(all); testDepositIntegralOverflowMPT(all - fixCleanup3_4_0); testWithdrawIntegralNoOverflowMPT(); + testDanglingAMMMPTokenFreezeCheck(); } }; From 820ca5b33201c67d290d5c16fa2419121ee76de0 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 17 Aug 2026 23:19:56 +0000 Subject: [PATCH 5/5] refactor: Convert boost::beast::string_view to std::string_view (#6306) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com> Co-authored-by: Mayukha Vadari Co-authored-by: Ayaz Salikhov Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> Co-authored-by: Mayukha Vadari Co-authored-by: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Co-authored-by: Cursor Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> --- include/xrpl/beast/rfc2616.h | 3 ++- include/xrpl/config/BasicConfig.h | 1 - include/xrpl/json/Output.h | 7 +++---- include/xrpl/server/detail/BaseWSPeer.h | 16 ++++++++-------- src/libxrpl/json/Writer.cpp | 5 +++-- src/xrpld/overlay/detail/ProtocolVersion.cpp | 5 +++-- src/xrpld/overlay/detail/ProtocolVersion.h | 7 +++---- src/xrpld/rpc/detail/ServerHandler.cpp | 6 +++--- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/include/xrpl/beast/rfc2616.h b/include/xrpl/beast/rfc2616.h index 0e061845fb..87d63b0260 100644 --- a/include/xrpl/beast/rfc2616.h +++ b/include/xrpl/beast/rfc2616.h @@ -11,6 +11,7 @@ #include #include #include +#include #include namespace beast::rfc2616 { @@ -186,7 +187,7 @@ splitCommas(FwdIt first, FwdIt last) template > Result -splitCommas(boost::beast::string_view const& s) +splitCommas(std::string_view s) { return splitCommas(s.begin(), s.end()); } diff --git a/include/xrpl/config/BasicConfig.h b/include/xrpl/config/BasicConfig.h index 607a0c3e5f..2278a0fa68 100644 --- a/include/xrpl/config/BasicConfig.h +++ b/include/xrpl/config/BasicConfig.h @@ -2,7 +2,6 @@ #include -#include #include #include diff --git a/include/xrpl/json/Output.h b/include/xrpl/json/Output.h index 53d453c277..f73bd38c77 100644 --- a/include/xrpl/json/Output.h +++ b/include/xrpl/json/Output.h @@ -1,20 +1,19 @@ #pragma once -#include - #include #include +#include namespace json { class Value; -using Output = std::function; +using Output = std::function; inline Output stringOutput(std::string& s) { - return [&](boost::beast::string_view const& b) { s.append(b.data(), b.size()); }; + return [&](std::string_view b) { s.append(b.data(), b.size()); }; } /** diff --git a/include/xrpl/server/detail/BaseWSPeer.h b/include/xrpl/server/detail/BaseWSPeer.h index b1670865bd..403d7f92ee 100644 --- a/include/xrpl/server/detail/BaseWSPeer.h +++ b/include/xrpl/server/detail/BaseWSPeer.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -62,8 +63,7 @@ private: bool pingActive_ = false; boost::beast::websocket::ping_data payload_; error_code ec_; - std::function - controlCallback_; + std::function controlCallback_; public: template @@ -151,7 +151,7 @@ protected: onPing(error_code const& ec); void - onPingPong(boost::beast::websocket::frame_type kind, boost::beast::string_view payload); + onPingPong(boost::beast::websocket::frame_type kind, std::string_view payload); void onTimer(error_code ec); @@ -189,9 +189,9 @@ BaseWSPeer::run() impl().ws_.set_option(port().pmdOptions); // Must manage the control callback memory outside of the `control_callback` // function - controlCallback_ = [this]( - boost::beast::websocket::frame_type kind, - boost::beast::string_view payload) { onPingPong(kind, payload); }; + controlCallback_ = [this](boost::beast::websocket::frame_type kind, std::string_view payload) { + onPingPong(kind, payload); + }; impl().ws_.control_callback(controlCallback_); startTimer(); closeOnTimer_ = true; @@ -430,11 +430,11 @@ template void BaseWSPeer::onPingPong( boost::beast::websocket::frame_type kind, - boost::beast::string_view payload) + std::string_view payload) { if (kind == boost::beast::websocket::frame_type::pong) { - boost::beast::string_view const p(payload_.begin()); + std::string_view const p(payload_.begin(), payload_.size()); if (payload == p) { closeOnTimer_ = false; diff --git a/src/libxrpl/json/Writer.cpp b/src/libxrpl/json/Writer.cpp index 4c922a0e33..c5ce4666ef 100644 --- a/src/libxrpl/json/Writer.cpp +++ b/src/libxrpl/json/Writer.cpp @@ -9,6 +9,7 @@ #include // IWYU pragma: keep #include #include +#include #include #include @@ -87,14 +88,14 @@ public: } void - output(boost::beast::string_view const& bytes) + output(std::string_view bytes) { markStarted(); output_(bytes); } void - stringOutput(boost::beast::string_view const& bytes) + stringOutput(std::string_view bytes) { markStarted(); std::size_t position = 0, writtenUntil = 0; diff --git a/src/xrpld/overlay/detail/ProtocolVersion.cpp b/src/xrpld/overlay/detail/ProtocolVersion.cpp index 1296041ad5..74dad61828 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.cpp +++ b/src/xrpld/overlay/detail/ProtocolVersion.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include namespace xrpl { @@ -52,7 +53,7 @@ to_string(ProtocolVersion const& p) } std::vector -parseProtocolVersions(boost::beast::string_view const& value) +parseProtocolVersions(std::string_view value) { static boost::regex const kRE( "^" // start of line @@ -119,7 +120,7 @@ negotiateProtocolVersion(std::vector const& versions) } std::optional -negotiateProtocolVersion(boost::beast::string_view const& versions) +negotiateProtocolVersion(std::string_view versions) { auto const them = parseProtocolVersions(versions); diff --git a/src/xrpld/overlay/detail/ProtocolVersion.h b/src/xrpld/overlay/detail/ProtocolVersion.h index b56871318a..5c05f63e2a 100644 --- a/src/xrpld/overlay/detail/ProtocolVersion.h +++ b/src/xrpld/overlay/detail/ProtocolVersion.h @@ -1,10 +1,9 @@ #pragma once -#include - #include #include #include +#include #include #include @@ -43,7 +42,7 @@ to_string(ProtocolVersion const& p); * no duplicates and will be sorted in ascending protocol order. */ std::vector -parseProtocolVersions(boost::beast::string_view const& s); +parseProtocolVersions(std::string_view s); /** * Given a list of supported protocol versions, choose the one we prefer. @@ -55,7 +54,7 @@ negotiateProtocolVersion(std::vector const& versions); * Given a list of supported protocol versions, choose the one we prefer. */ std::optional -negotiateProtocolVersion(boost::beast::string_view const& versions); +negotiateProtocolVersion(std::string_view versions); /** * The list of all the protocol versions we support. diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 827d8705fd..28e7eebd63 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -264,7 +264,7 @@ ServerHandler::onHandoff( static inline json::Output makeOutput(Session& session) { - return [&](boost::beast::string_view const& b) { session.write(b.data(), b.size()); }; + return [&](std::string_view b) { session.write(b.data(), b.size()); }; } static std::map @@ -564,11 +564,11 @@ ServerHandler::processSession( makeOutput(*session), coro, forwardedFor(session->request()), - [&] { + [&] -> std::string_view { auto const iter = session->request().find("X-User"); if (iter != session->request().end()) return iter->value(); - return boost::beast::string_view{}; + return {}; }()); if (beast::rfc2616::isKeepAlive(session->request()))