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");