Simplify SHAMapTreeNode APIs:

- Provide separate functions for serializing depending on whether
  one wants a "wire" version of a node, or one suitable for hashing.
- Remove unused functions

src/ripple/app/ledger/impl/InboundLedgers.cpp

@@ -254,7 +254,7 @@ public:
                     return;
 
                 s.erase();
-                newNode->addRaw(s, snfPREFIX);
+                newNode->serializeWithPrefix(s);
 
                 app_.getLedgerMaster().addFetchPack(
                     newNode->getNodeHash().as_uint256(),

src/ripple/overlay/impl/PeerImp.cpp

@@ -2734,24 +2734,21 @@ PeerImp::getLedger(std::shared_ptr<protocol::TMGetLedger> const& m)
             {
                 // return account state root node if possible
                 Serializer rootNode(768);
-                if (stateMap.getRootNode(rootNode, snfWIRE))
+
+                stateMap.serializeRoot(rootNode);
+                reply.add_nodes()->set_nodedata(
+                    rootNode.getDataPtr(), rootNode.getLength());
+
+                if (ledger->info().txHash != beast::zero)
                 {
-                    reply.add_nodes()->set_nodedata(
+                    auto const& txMap = ledger->txMap();
-                        rootNode.getDataPtr(), rootNode.getLength());
+                    if (txMap.getHash() != beast::zero)
-
-                    if (ledger->info().txHash != beast::zero)
                     {
-                        auto const& txMap = ledger->txMap();
+                        rootNode.erase();
 
-                        if (txMap.getHash() != beast::zero)
+                        txMap.serializeRoot(rootNode);
-                        {
+                        reply.add_nodes()->set_nodedata(
-                            rootNode.erase();
+                            rootNode.getDataPtr(), rootNode.getLength());
-
-                            if (txMap.getRootNode(rootNode, snfWIRE))
-                                reply.add_nodes()->set_nodedata(
-                                    rootNode.getDataPtr(),
-                                    rootNode.getLength());
-                        }
                     }
                 }
             }

src/ripple/shamap/SHAMap.h

@@ -256,8 +256,10 @@ public:
         bool fatLeaves,
         std::uint32_t depth) const;
 
-    bool
+    /** Serializes the root in a format appropriate for sending over the wire */
-    getRootNode(Serializer& s, SHANodeFormat format) const;
+    void
+    serializeRoot(Serializer& s) const;
+
     std::vector<uint256>
     getNeededHashes(int max, SHAMapSyncFilter* filter);
     SHAMapAddNode

src/ripple/shamap/SHAMapTreeNode.h

@@ -32,12 +32,6 @@
 
 namespace ripple {
 
-enum SHANodeFormat {
-    snfPREFIX = 1,  // Form that hashes to its official hash
-    snfWIRE = 2,    // Compressed form used on the wire
-    snfHASH = 3,    // just the hash
-};
-
 // A SHAMapHash is the hash of a node in a SHAMap, and also the
 // type of the hash of the entire SHAMap.
 class SHAMapHash
@@ -123,7 +117,6 @@ class SHAMapAbstractNode
 {
 public:
     enum TNType {
-        tnERROR = 0,
         tnINNER = 1,
         tnTRANSACTION_NM = 2,  // transaction, no metadata
         tnTRANSACTION_MD = 3,  // transaction, with metadata
@@ -158,14 +151,19 @@ public:
     bool
     isInner() const;
     bool
-    isValid() const;
-    bool
     isInBounds(SHAMapNodeID const& id) const;
 
     virtual bool
     updateHash() = 0;
+
+    /** Serialize the node in a format appropriate for sending over the wire */
     virtual void
-    addRaw(Serializer&, SHANodeFormat format) const = 0;
+    serializeForWire(Serializer&) const = 0;
+
+    /** Serialize the node in a format appropriate for hashing */
+    virtual void
+    serializeWithPrefix(Serializer&) const = 0;
+
     virtual std::string
     getString(SHAMapNodeID const&) const;
     virtual std::shared_ptr<SHAMapAbstractNode>
@@ -248,8 +246,13 @@ public:
     updateHash() override;
     void
     updateHashDeep();
+
     void
-    addRaw(Serializer&, SHANodeFormat format) const override;
+    serializeForWire(Serializer&) const override;
+
+    void
+    serializeWithPrefix(Serializer&) const override;
+
     std::string
     getString(SHAMapNodeID const&) const override;
     uint256 const&
@@ -293,7 +296,11 @@ public:
     clone(std::uint32_t seq) const override;
 
     void
-    addRaw(Serializer&, SHANodeFormat format) const override;
+    serializeForWire(Serializer&) const override;
+
+    void
+    serializeWithPrefix(Serializer&) const override;
+
     uint256 const&
     key() const override;
     void
@@ -366,12 +373,6 @@ SHAMapAbstractNode::isInner() const
     return mType == tnINNER;
 }
 
-inline bool
-SHAMapAbstractNode::isValid() const
-{
-    return mType != tnERROR;
-}
-
 inline bool
 SHAMapAbstractNode::isInBounds(SHAMapNodeID const& id) const
 {

src/ripple/shamap/impl/SHAMap.cpp

@@ -406,14 +406,12 @@ std::shared_ptr<Node>
 SHAMap::unshareNode(std::shared_ptr<Node> node, SHAMapNodeID const& nodeID)
 {
     // make sure the node is suitable for the intended operation (copy on write)
-    assert(node->isValid());
     assert(node->getSeq() <= seq_);
     if (node->getSeq() != seq_)
     {
         // have a CoW
         assert(state_ != SHAMapState::Immutable);
         node = std::static_pointer_cast<Node>(node->clone(seq_));
-        assert(node->isValid());
         if (nodeID.isRoot())
             root_ = node;
     }
@@ -772,13 +770,13 @@ SHAMap::addGiveItem(
 
         std::shared_ptr<SHAMapTreeNode> newNode =
             std::make_shared<SHAMapTreeNode>(std::move(item), type, seq_);
-        assert(newNode->isValid() && newNode->isLeaf());
+        assert(newNode->isLeaf());
         auto inner = std::static_pointer_cast<SHAMapInnerNode>(node);
         inner->setChild(b1, newNode);
 
         newNode =
             std::make_shared<SHAMapTreeNode>(std::move(otherItem), type, seq_);
-        assert(newNode->isValid() && newNode->isLeaf());
+        assert(newNode->isLeaf());
         inner->setChild(b2, newNode);
     }
 
@@ -907,7 +905,7 @@ SHAMap::writeNode(
     canonicalize(node->getNodeHash(), node);
 
     Serializer s;
-    node->addRaw(s, snfPREFIX);
+    node->serializeWithPrefix(s);
     f_.db().store(
         t,
         std::move(s.modData()),

src/ripple/shamap/impl/SHAMapSync.cpp

@@ -39,9 +39,6 @@ void
 SHAMap::visitNodes(
     std::function<bool(SHAMapAbstractNode&)> const& function) const
 {
-    // Visit every node in a SHAMap
-    assert(root_->isValid());
-
     if (!root_)
         return;
 
@@ -108,8 +105,6 @@ SHAMap::visitDifferences(
 {
     // Visit every node in this SHAMap that is not present
     // in the specified SHAMap
-    assert(root_->isValid());
-
     if (!root_)
         return;
 
@@ -320,7 +315,6 @@ SHAMap::gmn_ProcessDeferredReads(MissingNodes& mn)
 std::vector<std::pair<SHAMapNodeID, uint256>>
 SHAMap::getMissingNodes(int max, SHAMapSyncFilter* filter)
 {
-    assert(root_->isValid());
     assert(root_->getNodeHash().isNonZero());
     assert(max > 0);
 
@@ -493,7 +487,7 @@ SHAMap::getNodeFat(
         {
             // Add this node to the reply
             Serializer s;
-            node->addRaw(s, snfWIRE);
+            node->serializeForWire(s);
             nodeIDs.push_back(nodeID);
             rawNodes.push_back(std::move(s.modData()));
         }
@@ -527,7 +521,7 @@ SHAMap::getNodeFat(
                         {
                             // Just include this node
                             Serializer ns;
-                            childNode->addRaw(ns, snfWIRE);
+                            childNode->serializeForWire(ns);
                             nodeIDs.push_back(childID);
                             rawNodes.push_back(std::move(ns.modData()));
                         }
@@ -540,11 +534,10 @@ SHAMap::getNodeFat(
     return true;
 }
 
-bool
+void
-SHAMap::getRootNode(Serializer& s, SHANodeFormat format) const
+SHAMap::serializeRoot(Serializer& s) const
 {
-    root_->addRaw(s, format);
+    root_->serializeForWire(s);
-    return true;
 }
 
 SHAMapAddNode
@@ -563,7 +556,7 @@ SHAMap::addRootNode(
 
     assert(seq_ >= 1);
     auto node = SHAMapAbstractNode::makeFromWire(rootNode);
-    if (!node || !node->isValid() || node->getNodeHash() != hash)
+    if (!node || node->getNodeHash() != hash)
         return SHAMapAddNode::invalid();
 
     if (backed_)
@@ -577,7 +570,7 @@ SHAMap::addRootNode(
     if (filter)
     {
         Serializer s;
-        root_->addRaw(s, snfPREFIX);
+        root_->serializeWithPrefix(s);
         filter->gotNode(
             false,
             root_->getNodeHash(),
@@ -633,8 +626,7 @@ SHAMap::addKnownNode(
 
         if (iNode == nullptr)
         {
-            if (!newNode || !newNode->isValid() ||
+            if (!newNode || childHash != newNode->getNodeHash())
-                childHash != newNode->getNodeHash())
             {
                 JLOG(journal_.warn()) << "Corrupt node received";
                 return SHAMapAddNode::invalid();
@@ -665,7 +657,7 @@ SHAMap::addKnownNode(
             if (filter)
             {
                 Serializer s;
-                newNode->addRaw(s, snfPREFIX);
+                newNode->serializeWithPrefix(s);
                 filter->gotNode(
                     false,
                     childHash,
@@ -827,7 +819,7 @@ SHAMap::getFetchPack(
             if (includeLeaves || smn.isInner())
             {
                 Serializer s;
-                smn.addRaw(s, snfPREFIX);
+                smn.serializeWithPrefix(s);
                 func(smn.getNodeHash(), s.peekData());
 
                 if (--max <= 0)

src/ripple/shamap/impl/SHAMapTreeNode.cpp

@@ -31,6 +31,14 @@
 
 namespace ripple {
 
+// These are wire-protocol identifiers used during serialization to encode the
+// type of a node. They should not be arbitrarily be changed.
+static constexpr unsigned char const wireTypeTransaction = 0;
+static constexpr unsigned char const wireTypeAccountState = 1;
+static constexpr unsigned char const wireTypeInner = 2;
+static constexpr unsigned char const wireTypeCompressedInner = 3;
+static constexpr unsigned char const wireTypeTransactionWithMeta = 4;
+
 std::mutex SHAMapInnerNode::childLock;
 
 SHAMapAbstractNode::~SHAMapAbstractNode() = default;
@@ -235,19 +243,19 @@ SHAMapAbstractNode::makeFromWire(Slice rawNode)
 
     std::uint32_t const seq = 0;
 
-    if (type == 0)
+    if (type == wireTypeTransaction)
         return makeTransaction(rawNode, seq, hash, hashValid);
 
-    if (type == 1)
+    if (type == wireTypeAccountState)
         return makeAccountState(rawNode, seq, hash, hashValid);
 
-    if (type == 2)
+    if (type == wireTypeInner)
         return SHAMapInnerNode::makeFullInner(rawNode, seq, hash, hashValid);
 
-    if (type == 3)
+    if (type == wireTypeCompressedInner)
         return SHAMapInnerNode::makeCompressedInner(rawNode, seq);
 
-    if (type == 4)
+    if (type == wireTypeTransactionWithMeta)
         return makeTransactionWithMeta(rawNode, seq, hash, hashValid);
 
     Throw<std::runtime_error>(
@@ -351,112 +359,88 @@ SHAMapTreeNode::updateHash()
 }
 
 void
-SHAMapInnerNode::addRaw(Serializer& s, SHANodeFormat format) const
+SHAMapInnerNode::serializeForWire(Serializer& s) const
 {
-    assert((format == snfPREFIX) || (format == snfWIRE) || (format == snfHASH));
+    assert(mType == tnINNER);
+    assert(!isEmpty());
 
-    if (mType == tnERROR)
+    // If the node is sparse, then only send non-empty branches:
-        Throw<std::runtime_error>("invalid I node type");
+    if (getBranchCount() < 12)
-
-    if (format == snfHASH)
     {
-        s.addBitString(mHash.as_uint256());
+        // compressed node
-    }
+        for (int i = 0; i < mHashes.size(); ++i)
-    else if (mType == tnINNER)
-    {
-        assert(!isEmpty());
-
-        if (format == snfPREFIX)
         {
-            s.add32(HashPrefix::innerNode);
+            if (!isEmptyBranch(i))
-
-            for (auto const& hh : mHashes)
-                s.addBitString(hh.as_uint256());
-        }
-        else  // format == snfWIRE
-        {
-            if (getBranchCount() < 12)
             {
-                // compressed node
+                s.addBitString(mHashes[i].as_uint256());
-                for (int i = 0; i < mHashes.size(); ++i)
+                s.add8(i);
-                    if (!isEmptyBranch(i))
-                    {
-                        s.addBitString(mHashes[i].as_uint256());
-                        s.add8(i);
-                    }
-
-                s.add8(3);
-            }
-            else
-            {
-                for (auto const& hh : mHashes)
-                    s.addBitString(hh.as_uint256());
-
-                s.add8(2);
             }
         }
+
+        s.add8(wireTypeCompressedInner);
     }
     else
-        assert(false);
+    {
+        for (auto const& hh : mHashes)
+            s.addBitString(hh.as_uint256());
+
+        s.add8(wireTypeInner);
+    }
 }
 
 void
-SHAMapTreeNode::addRaw(Serializer& s, SHANodeFormat format) const
+SHAMapInnerNode::serializeWithPrefix(Serializer& s) const
 {
-    assert((format == snfPREFIX) || (format == snfWIRE) || (format == snfHASH));
+    assert(mType == tnINNER);
+    assert(!isEmpty());
 
-    if (mType == tnERROR)
+    s.add32(HashPrefix::innerNode);
-        Throw<std::runtime_error>("invalid I node type");
+    for (auto const& hh : mHashes)
+        s.addBitString(hh.as_uint256());
+}
 
-    if (format == snfHASH)
+void
+SHAMapTreeNode::serializeForWire(Serializer& s) const
+{
+    if (mType == tnACCOUNT_STATE)
     {
-        s.addBitString(mHash.as_uint256());
+        s.addRaw(mItem->peekData());
-    }
+        s.addBitString(mItem->key());
-    else if (mType == tnACCOUNT_STATE)
+        s.add8(wireTypeAccountState);
-    {
-        if (format == snfPREFIX)
-        {
-            s.add32(HashPrefix::leafNode);
-            s.addRaw(mItem->peekData());
-            s.addBitString(mItem->key());
-        }
-        else
-        {
-            s.addRaw(mItem->peekData());
-            s.addBitString(mItem->key());
-            s.add8(1);
-        }
     }
     else if (mType == tnTRANSACTION_NM)
     {
-        if (format == snfPREFIX)
+        s.addRaw(mItem->peekData());
-        {
+        s.add8(wireTypeTransaction);
-            s.add32(HashPrefix::transactionID);
-            s.addRaw(mItem->peekData());
-        }
-        else
-        {
-            s.addRaw(mItem->peekData());
-            s.add8(0);
-        }
     }
     else if (mType == tnTRANSACTION_MD)
     {
-        if (format == snfPREFIX)
+        s.addRaw(mItem->peekData());
-        {
+        s.addBitString(mItem->key());
-            s.add32(HashPrefix::txNode);
+        s.add8(wireTypeTransactionWithMeta);
-            s.addRaw(mItem->peekData());
+    }
-            s.addBitString(mItem->key());
+}
-        }
+
-        else
+void
-        {
+SHAMapTreeNode::serializeWithPrefix(Serializer& s) const
-            s.addRaw(mItem->peekData());
+{
-            s.addBitString(mItem->key());
+    if (mType == tnACCOUNT_STATE)
-            s.add8(4);
+    {
-        }
+        s.add32(HashPrefix::leafNode);
+        s.addRaw(mItem->peekData());
+        s.addBitString(mItem->key());
+    }
+    else if (mType == tnTRANSACTION_NM)
+    {
+        s.add32(HashPrefix::transactionID);
+        s.addRaw(mItem->peekData());
+    }
+    else if (mType == tnTRANSACTION_MD)
+    {
+        s.add32(HashPrefix::txNode);
+        s.addRaw(mItem->peekData());
+        s.addBitString(mItem->key());
     }
-    else
-        assert(false);
 }
 
 bool

src/test/nodestore/DatabaseShard_test.cpp

@@ -285,7 +285,7 @@ class DatabaseShard_test : public TestBase
         // Store the state map
         auto visitAcc = [&](SHAMapAbstractNode& node) {
             Serializer s;
-            node.addRaw(s, snfPREFIX);
+            node.serializeWithPrefix(s);
             db.store(
                 node.getType() == SHAMapAbstractNode::TNType::tnINNER
                     ? hotUNKNOWN
@@ -313,7 +313,7 @@ class DatabaseShard_test : public TestBase
         // Store the transaction map
         auto visitTx = [&](SHAMapAbstractNode& node) {
             Serializer s;
-            node.addRaw(s, snfPREFIX);
+            node.serializeWithPrefix(s);
             db.store(
                 node.getType() == SHAMapAbstractNode::TNType::tnINNER
                     ? hotUNKNOWN
@@ -359,7 +359,7 @@ class DatabaseShard_test : public TestBase
         // walk shamap and validate each node
         auto fcompAcc = [&](SHAMapAbstractNode& node) -> bool {
             Serializer s;
-            node.addRaw(s, snfPREFIX);
+            node.serializeWithPrefix(s);
             auto nSrc{NodeObject::createObject(
                 node.getType() == SHAMapAbstractNode::TNType::tnINNER
                     ? hotUNKNOWN
@@ -383,7 +383,7 @@ class DatabaseShard_test : public TestBase
 
         auto fcompTx = [&](SHAMapAbstractNode& node) -> bool {
             Serializer s;
-            node.addRaw(s, snfPREFIX);
+            node.serializeWithPrefix(s);
             auto nSrc{NodeObject::createObject(
                 node.getType() == SHAMapAbstractNode::TNType::tnINNER
                     ? hotUNKNOWN