Eliminate redundant traversal logic of SHAMap:

*  Only the const_iterator interface remains.

src/ripple/app/ledger/Ledger.cpp

@@ -894,9 +894,8 @@ boost::optional<uint256>
 Ledger::succ (uint256 const& key,
     boost::optional<uint256> last) const
 {
-    auto const item =
+    auto item = stateMap_->upper_bound(key);
-        stateMap_->peekNextItem(key);
+    if (item == stateMap_->end())
-    if (! item)
         return boost::none;
     if (last && item->key() >= last)
         return boost::none;
@@ -1148,16 +1147,6 @@ void Ledger::visitStateItems (std::function<void (SLE::ref)> callback) const
     }
 }
 
-uint256 Ledger::getNextLedgerIndex (uint256 const& hash,
-    boost::optional<uint256> const& last) const
-{
-    auto const node =
-        stateMap_->peekNextItem(hash);
-    if ((! node) || (last && node->key() >= *last))
-        return {};
-    return node->key();
-}
-
 bool Ledger::walkLedger () const
 {
     std::vector <SHAMapMissingNode> missingNodes1;

src/ripple/app/ledger/Ledger.h

@@ -317,10 +317,6 @@ public:
 
     bool pendSaveValidated (bool isSynchronous, bool isCurrent);
 
-    // first node >hash, <last
-    uint256 getNextLedgerIndex (uint256 const& hash,
-        boost::optional<uint256> const& last = boost::none) const;
-
     std::vector<uint256> getNeededTransactionHashes (
         int max, SHAMapSyncFilter* filter) const;
 

src/ripple/shamap/SHAMap.h

@@ -165,16 +165,7 @@ public:
         peekItem (uint256 const& id, SHAMapTreeNode::TNType & type) const;
 
     // traverse functions
-    std::shared_ptr<SHAMapItem const> const& peekFirstItem () const;
+    const_iterator upper_bound(uint256 const& id) const;
-    std::shared_ptr<SHAMapItem const> const&
-        peekFirstItem (SHAMapTreeNode::TNType & type) const;
-    SHAMapItem const* peekFirstItem(NodeStack& stack) const;
-    std::shared_ptr<SHAMapItem const> const& peekLastItem () const;
-    std::shared_ptr<SHAMapItem const> const& peekNextItem (uint256 const& ) const;
-    std::shared_ptr<SHAMapItem const> const&
-        peekNextItem (uint256 const& , SHAMapTreeNode::TNType & type) const;
-    SHAMapItem const* peekNextItem(uint256 const& id, NodeStack& stack) const;
-    std::shared_ptr<SHAMapItem const> const& peekPrevItem (uint256 const& ) const;
 
     void visitNodes (std::function<bool (SHAMapAbstractNode&)> const&) const;
     void
@@ -275,9 +266,7 @@ private:
         writeNode(NodeObjectType t, std::uint32_t seq,
                   std::shared_ptr<SHAMapAbstractNode> node) const;
 
-    SHAMapTreeNode* firstBelow (SHAMapAbstractNode*) const;
     SHAMapTreeNode* firstBelow (SHAMapAbstractNode*, NodeStack& stack) const;
-    SHAMapTreeNode* lastBelow (SHAMapAbstractNode*) const;
 
     // Simple descent
     // Get a child of the specified node
@@ -305,6 +294,8 @@ private:
     bool hasInnerNode (SHAMapNodeID const& nodeID, uint256 const& hash) const;
     bool hasLeafNode (uint256 const& tag, uint256 const& hash) const;
 
+    SHAMapItem const* peekFirstItem(NodeStack& stack) const;
+    SHAMapItem const* peekNextItem(uint256 const& id, NodeStack& stack) const;
     bool walkBranch (SHAMapAbstractNode* node,
                      std::shared_ptr<SHAMapItem const> const& otherMapItem,
                      bool isFirstMap, Delta & differences, int & maxCount) const;
@@ -389,6 +380,7 @@ public:
 private:
     explicit const_iterator(SHAMap const* map);
     const_iterator(SHAMap const* map, pointer item);
+    const_iterator(SHAMap const* map, pointer item, NodeStack&& stack);
 
     friend bool operator==(const_iterator const& x, const_iterator const& y);
     friend class SHAMap;
@@ -408,6 +400,15 @@ SHAMap::const_iterator::const_iterator(SHAMap const* map, pointer item)
 {
 }
 
+inline
+SHAMap::const_iterator::const_iterator(SHAMap const* map, pointer item,
+                                       NodeStack&& stack)
+    : stack_(std::move(stack))
+    , map_(map)
+    , item_(item)
+{
+}
+
 inline
 SHAMap::const_iterator::reference
 SHAMap::const_iterator::operator*() const

src/ripple/shamap/impl/SHAMap.cpp

@@ -149,7 +149,6 @@ SHAMapTreeNode* SHAMap::walkToPointer (uint256 const& id) const
 {
     auto inNode = root_.get();
     SHAMapNodeID nodeID;
-    uint256 nodeHash;
 
     while (inNode->isInner ())
     {
@@ -416,35 +415,6 @@ SHAMap::unshareNode (std::shared_ptr<Node> node, SHAMapNodeID const& nodeID)
     return node;
 }
 
-SHAMapTreeNode*
-SHAMap::firstBelow (SHAMapAbstractNode* node) const
-{
-    // Return the first item below this node
-    do
-    {
-        assert(node != nullptr);
-
-        if (node->isLeaf ())
-            return static_cast<SHAMapTreeNode*>(node);
-
-        // Walk down the tree
-        bool foundNode = false;
-        auto inner = static_cast<SHAMapInnerNode*>(node);
-        for (int i = 0; i < 16; ++i)
-        {
-            if (!inner->isEmptyBranch (i))
-            {
-                node = descendThrow (inner, i);
-                foundNode = true;
-                break;
-            }
-        }
-        if (!foundNode)
-            return nullptr;
-    }
-    while (true);
-}
-
 SHAMapTreeNode*
 SHAMap::firstBelow(SHAMapAbstractNode* node, NodeStack& stack) const
 {
@@ -472,32 +442,6 @@ SHAMap::firstBelow(SHAMapAbstractNode* node, NodeStack& stack) const
     }
 }
 
-SHAMapTreeNode*
-SHAMap::lastBelow (SHAMapAbstractNode* node) const
-{
-    do
-    {
-        if (node->isLeaf ())
-            return static_cast<SHAMapTreeNode*>(node);
-
-        // Walk down the tree
-        bool foundNode = false;
-        auto inner = static_cast<SHAMapInnerNode*>(node);
-        for (int i = 15; i >= 0; --i)
-        {
-            if (!inner->isEmptyBranch (i))
-            {
-                node = descendThrow (inner, i);
-                foundNode = true;
-                break;
-            }
-        }
-        if (!foundNode)
-            return nullptr;
-    }
-    while (true);
-}
-
 static const std::shared_ptr<SHAMapItem const> no_item;
 
 std::shared_ptr<SHAMapItem const> const&
@@ -550,17 +494,6 @@ SHAMap::fetch (uint256 const& key) const
     return leaf->peekItem();
 }
 
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekFirstItem () const
-{
-    SHAMapTreeNode* node = firstBelow (root_.get ());
-
-    if (!node)
-        return no_item;
-
-    return node->peekItem ();
-}
-
 SHAMapItem const*
 SHAMap::peekFirstItem(NodeStack& stack) const
 {
@@ -572,81 +505,6 @@ SHAMap::peekFirstItem(NodeStack& stack) const
     return node->peekItem().get();
 }
 
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekFirstItem (SHAMapTreeNode::TNType& type) const
-{
-    SHAMapTreeNode* node = firstBelow (root_.get ());
-
-    if (!node)
-        return no_item;
-
-    type = node->getType ();
-    return node->peekItem ();
-}
-
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekLastItem () const
-{
-    SHAMapTreeNode* node = lastBelow (root_.get ());
-
-    if (!node)
-        return no_item;
-
-    return node->peekItem ();
-}
-
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekNextItem (uint256 const& id) const
-{
-    SHAMapTreeNode::TNType type;
-    return peekNextItem (id, type);
-}
-
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekNextItem (uint256 const& id, SHAMapTreeNode::TNType& type) const
-{
-    // Get a pointer to the next item in the tree after a given item - item need not be in tree
-
-    auto stack = getStack (id, true);
-
-    while (!stack.empty ())
-    {
-        auto node = stack.top().first.get();
-        auto nodeID = stack.top().second;
-        stack.pop ();
-
-        if (node->isLeaf ())
-        {
-            auto leaf = static_cast<SHAMapTreeNode*>(node);
-            if (leaf->peekItem ()->key() > id)
-            {
-                type = leaf->getType ();
-                return leaf->peekItem ();
-            }
-        }
-        else
-        {
-            // breadth-first
-            auto inner = static_cast<SHAMapInnerNode*>(node);
-            for (int i = nodeID.selectBranch (id) + 1; i < 16; ++i)
-                if (!inner->isEmptyBranch (i))
-                {
-                    node = descendThrow (inner, i);
-                    auto leaf = firstBelow (node);
-
-                    if (!leaf)
-                        throw SHAMapMissingNode(type_, id);
-
-                    type = leaf->getType ();
-                    return leaf->peekItem ();
-                }
-        }
-    }
-
-    // must be last item
-    return no_item;
-}
-
 SHAMapItem const*
 SHAMap::peekNextItem(uint256 const& id, NodeStack& stack) const
 {
@@ -679,43 +537,6 @@ SHAMap::peekNextItem(uint256 const& id, NodeStack& stack) const
     return nullptr;
 }
 
-// Get a pointer to the previous item in the tree after a given item - item need not be in tree
-std::shared_ptr<SHAMapItem const> const&
-SHAMap::peekPrevItem (uint256 const& id) const
-{
-    auto stack = getStack (id, true);
-
-    while (!stack.empty ())
-    {
-        auto node = stack.top ().first.get();
-        auto nodeID = stack.top ().second;
-        stack.pop ();
-
-        if (node->isLeaf ())
-        {
-            auto leaf = static_cast<SHAMapTreeNode*>(node);
-            if (leaf->peekItem ()->key() < id)
-                return leaf->peekItem ();
-        }
-        else
-        {
-            auto inner = static_cast<SHAMapInnerNode*>(node);
-            for (int i = nodeID.selectBranch (id) - 1; i >= 0; --i)
-            {
-                if (!inner->isEmptyBranch (i))
-                {
-                    node = descendThrow (inner, i);
-                    auto leaf = lastBelow (node);
-                    return leaf->peekItem ();
-                }
-            }
-        }
-    }
-
-    // must be first item
-    return no_item;
-}
-
 std::shared_ptr<SHAMapItem const> const&
 SHAMap::peekItem (uint256 const& id) const
 {
@@ -751,6 +572,62 @@ SHAMap::peekItem (uint256 const& id, uint256& hash) const
     return leaf->peekItem ();
 }
 
+SHAMap::const_iterator
+SHAMap::upper_bound(uint256 const& id) const
+{
+    // Get a const_iterator to the next item in the tree after a given item
+    // item need not be in tree
+    NodeStack stack;
+    auto node = root_.get();
+    auto nodeID = SHAMapNodeID{};
+    auto foundLeaf = true;
+    while (!node->isLeaf())
+    {
+        stack.push ({node, nodeID});
+        auto branch = nodeID.selectBranch(id);
+        auto inner = static_cast<SHAMapInnerNode*>(node);
+        if (inner->isEmptyBranch(branch))
+        {
+            foundLeaf = false;
+            break;
+        }
+        node = descendThrow(inner, branch);
+        nodeID = nodeID.getChildNodeID(branch);
+    }
+    if (foundLeaf)
+        stack.push({node, nodeID});
+    while (!stack.empty())
+    {
+        std::tie(node, nodeID) = stack.top();
+        if (node->isLeaf())
+        {
+            auto leaf = static_cast<SHAMapTreeNode*>(node);
+            if (leaf->peekItem()->key() > id)
+                return const_iterator(this, leaf->peekItem().get(), std::move(stack));
+            stack.pop();
+        }
+        else
+        {
+            stack.pop();
+            auto inner = static_cast<SHAMapInnerNode*>(node);
+            for (auto branch = nodeID.selectBranch(id) + 1; branch < 16; ++branch)
+            {
+                if (!inner->isEmptyBranch(branch))
+                {
+                    node = descendThrow(inner, branch);
+                    nodeID = nodeID.getChildNodeID(branch);
+                    stack.push({node, nodeID});
+                    auto leaf = firstBelow(node, stack);
+                    if (!leaf)
+                        throw SHAMapMissingNode(type_, id);
+                    return const_iterator(this, leaf->peekItem().get(),
+                                          std::move(stack));
+                }
+            }
+        }
+    }
+    return end();
+}
 
 bool SHAMap::hasItem (uint256 const& id) const
 {

src/ripple/shamap/tests/SHAMap.test.cpp

@@ -68,31 +68,32 @@ public:
         unexpected (!sMap.addItem (i2, true, false), "no add");
         unexpected (!sMap.addItem (i1, true, false), "no add");
 
-        std::shared_ptr<SHAMapItem const> i;
+        auto i = sMap.begin();
-        i = sMap.peekFirstItem ();
+        auto e = sMap.end();
-        unexpected (!i || (*i != i1), "bad traverse");
+        unexpected (i == e || (*i != i1), "bad traverse");
-        i = sMap.peekNextItem (i->key());
+        ++i;
-        unexpected (!i || (*i != i2), "bad traverse");
+        unexpected (i == e || (*i != i2), "bad traverse");
-        i = sMap.peekNextItem (i->key());
+        ++i;
-        unexpected (i, "bad traverse");
+        unexpected (i != e, "bad traverse");
         sMap.addItem (i4, true, false);
         sMap.delItem (i2.key());
         sMap.addItem (i3, true, false);
-        i = sMap.peekFirstItem ();
+        i = sMap.begin();
-        unexpected (!i || (*i != i1), "bad traverse");
+        e = sMap.end();
-        i = sMap.peekNextItem (i->key());
+        unexpected (i == e || (*i != i1), "bad traverse");
-        unexpected (!i || (*i != i3), "bad traverse");
+        ++i;
-        i = sMap.peekNextItem (i->key());
+        unexpected (i == e || (*i != i3), "bad traverse");
-        unexpected (!i || (*i != i4), "bad traverse");
+        ++i;
-        i = sMap.peekNextItem (i->key());
+        unexpected (i == e || (*i != i4), "bad traverse");
-        unexpected (i, "bad traverse");
+        ++i;
+        unexpected (i != e, "bad traverse");
 
         testcase ("snapshot");
         uint256 mapHash = sMap.getHash ();
         std::shared_ptr<SHAMap> map2 = sMap.snapShot (false);
         unexpected (sMap.getHash () != mapHash, "bad snapshot");
         unexpected (map2->getHash () != mapHash, "bad snapshot");
-        unexpected (!sMap.delItem (sMap.peekFirstItem ()->key()), "bad mod");
+        unexpected (!sMap.delItem (sMap.begin()->key()), "bad mod");
         unexpected (sMap.getHash () == mapHash, "bad snapshot");
         unexpected (map2->getHash () != mapHash, "bad snapshot");