Cleanup SHAMap and simplify interfaces:

* Improve error reporting (more readable exception messages)
* Reduce function complexity (split oversized function to smaller pieces)
* Reduce code duplication
* Reduce buffer copying

src/ripple/app/ledger/InboundLedger.h

@@ -174,23 +174,13 @@ private:
 
     bool
     takeHeader(std::string const& data);
-    bool
-    takeTxNode(
-        const std::vector<SHAMapNodeID>& IDs,
-        const std::vector<Blob>& data,
-        SHAMapAddNode&);
+
+    void
+    receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode&);
+
     bool
     takeTxRootNode(Slice const& data, SHAMapAddNode&);
 
-    // VFALCO TODO Rename to receiveAccountStateNode
-    //             Don't use acronyms, but if we are going to use them at least
-    //             capitalize them correctly.
-    //
-    bool
-    takeAsNode(
-        const std::vector<SHAMapNodeID>& IDs,
-        const std::vector<Blob>& data,
-        SHAMapAddNode&);
     bool
     takeAsRootNode(Slice const& data, SHAMapAddNode&);
 

src/ripple/app/ledger/LedgerMaster.h

@@ -183,11 +183,6 @@ public:
     void
     setLedgerRangePresent(std::uint32_t minV, std::uint32_t maxV);
 
-    boost::optional<LedgerHash>
-    getLedgerHash(
-        std::uint32_t desiredSeq,
-        std::shared_ptr<ReadView const> const& knownGoodLedger);
-
     boost::optional<NetClock::time_point>
     getCloseTimeBySeq(LedgerIndex ledgerIndex);
 
@@ -264,7 +259,7 @@ public:
     gotFetchPack(bool progress, std::uint32_t seq);
 
     void
-    addFetchPack(uint256 const& hash, std::shared_ptr<Blob>& data);
+    addFetchPack(uint256 const& hash, std::shared_ptr<Blob> data);
 
     boost::optional<Blob>
     getFetchPack(uint256 const& hash) override;

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

@@ -47,7 +47,7 @@ enum {
     ,
     peerCountAdd = 2
 
-    // how many timeouts before we giveup
+    // how many timeouts before we give up
     ,
     ledgerTimeoutRetriesMax = 10
 
@@ -876,149 +876,87 @@ InboundLedger::takeHeader(std::string const& data)
     return true;
 }
 
-/** Process TX data received from a peer
+/** Process node data received from a peer
     Call with a lock
 */
-bool
-InboundLedger::takeTxNode(
-    const std::vector<SHAMapNodeID>& nodeIDs,
-    const std::vector<Blob>& data,
-    SHAMapAddNode& san)
+void
+InboundLedger::receiveNode(protocol::TMLedgerData& packet, SHAMapAddNode& san)
 {
     if (!mHaveHeader)
     {
-        JLOG(m_journal.warn()) << "TX node without header";
+        JLOG(m_journal.warn()) << "Missing ledger header";
         san.incInvalid();
-        return false;
+        return;
     }
-
-    if (mHaveTransactions || mFailed)
+    if (packet.type() == protocol::liTX_NODE)
+    {
+        if (mHaveTransactions || mFailed)
+        {
+            san.incDuplicate();
+            return;
+        }
+    }
+    else if (mHaveState || mFailed)
     {
         san.incDuplicate();
-        return true;
+        return;
     }
 
-    auto nodeIDit = nodeIDs.cbegin();
-    auto nodeDatait = data.begin();
-    TransactionStateSF filter(
-        mLedger->txMap().family().db(), app_.getLedgerMaster());
+    auto [map, filter] =
+        [&]() -> std::pair<SHAMap&, std::unique_ptr<SHAMapSyncFilter>> {
+        if (packet.type() == protocol::liTX_NODE)
+            return {
+                mLedger->txMap(),
+                std::make_unique<TransactionStateSF>(
+                    mLedger->txMap().family().db(), app_.getLedgerMaster())};
+        return {
+            mLedger->stateMap(),
+            std::make_unique<AccountStateSF>(
+                mLedger->stateMap().family().db(), app_.getLedgerMaster())};
+    }();
 
-    while (nodeIDit != nodeIDs.cend())
+    try
     {
-        if (nodeIDit->isRoot())
+        for (auto const& node : packet.nodes())
         {
-            san += mLedger->txMap().addRootNode(
-                SHAMapHash{mLedger->info().txHash},
-                makeSlice(*nodeDatait),
-                snfWIRE,
-                &filter);
+            SHAMapNodeID const nodeID(
+                node.nodeid().data(), node.nodeid().size());
+            if (nodeID.isRoot())
+                san += map.addRootNode(
+                    SHAMapHash{mLedger->info().accountHash},
+                    makeSlice(node.nodedata()),
+                    filter.get());
+            else
+                san += map.addKnownNode(
+                    nodeID, makeSlice(node.nodedata()), filter.get());
+
             if (!san.isGood())
-                return false;
+            {
+                JLOG(m_journal.warn()) << "Received bad node data";
+                return;
+            }
         }
+    }
+    catch (std::exception const& e)
+    {
+        JLOG(m_journal.error()) << "Received bad node data: " << e.what();
+        san.incInvalid();
+        return;
+    }
+
+    if (!map.isSynching())
+    {
+        if (packet.type() == protocol::liTX_NODE)
+            mHaveTransactions = true;
         else
-        {
-            san += mLedger->txMap().addKnownNode(
-                *nodeIDit, makeSlice(*nodeDatait), &filter);
-            if (!san.isGood())
-                return false;
-        }
+            mHaveState = true;
 
-        ++nodeIDit;
-        ++nodeDatait;
-    }
-
-    if (!mLedger->txMap().isSynching())
-    {
-        mHaveTransactions = true;
-
-        if (mHaveState)
+        if (mHaveTransactions && mHaveState)
         {
             mComplete = true;
             done();
         }
     }
-
-    return true;
-}
-
-/** Process AS data received from a peer
-    Call with a lock
-*/
-bool
-InboundLedger::takeAsNode(
-    const std::vector<SHAMapNodeID>& nodeIDs,
-    const std::vector<Blob>& data,
-    SHAMapAddNode& san)
-{
-    JLOG(m_journal.trace())
-        << "got ASdata (" << nodeIDs.size() << ") acquiring ledger " << mHash;
-    if (nodeIDs.size() == 1)
-    {
-        JLOG(m_journal.trace()) << "got AS node: " << nodeIDs.front();
-    }
-
-    ScopedLockType sl(mLock);
-
-    if (!mHaveHeader)
-    {
-        JLOG(m_journal.warn()) << "Don't have ledger header";
-        san.incInvalid();
-        return false;
-    }
-
-    if (mHaveState || mFailed)
-    {
-        san.incDuplicate();
-        return true;
-    }
-
-    auto nodeIDit = nodeIDs.cbegin();
-    auto nodeDatait = data.begin();
-    AccountStateSF filter(
-        mLedger->stateMap().family().db(), app_.getLedgerMaster());
-
-    while (nodeIDit != nodeIDs.cend())
-    {
-        if (nodeIDit->isRoot())
-        {
-            san += mLedger->stateMap().addRootNode(
-                SHAMapHash{mLedger->info().accountHash},
-                makeSlice(*nodeDatait),
-                snfWIRE,
-                &filter);
-            if (!san.isGood())
-            {
-                JLOG(m_journal.warn()) << "Bad ledger header";
-                return false;
-            }
-        }
-        else
-        {
-            san += mLedger->stateMap().addKnownNode(
-                *nodeIDit, makeSlice(*nodeDatait), &filter);
-            if (!san.isGood())
-            {
-                JLOG(m_journal.warn()) << "Unable to add AS node";
-                return false;
-            }
-        }
-
-        ++nodeIDit;
-        ++nodeDatait;
-    }
-
-    if (!mLedger->stateMap().isSynching())
-    {
-        mHaveState = true;
-
-        if (mHaveTransactions)
-        {
-            mComplete = true;
-            done();
-        }
-    }
-
-    return true;
 }
 
 /** Process AS root node received from a peer
@@ -1042,7 +980,7 @@ InboundLedger::takeAsRootNode(Slice const& data, SHAMapAddNode& san)
     AccountStateSF filter(
         mLedger->stateMap().family().db(), app_.getLedgerMaster());
     san += mLedger->stateMap().addRootNode(
-        SHAMapHash{mLedger->info().accountHash}, data, snfWIRE, &filter);
+        SHAMapHash{mLedger->info().accountHash}, data, &filter);
     return san.isGood();
 }
 
@@ -1067,7 +1005,7 @@ InboundLedger::takeTxRootNode(Slice const& data, SHAMapAddNode& san)
     TransactionStateSF filter(
         mLedger->txMap().family().db(), app_.getLedgerMaster());
     san += mLedger->txMap().addRootNode(
-        SHAMapHash{mLedger->info().txHash}, data, snfWIRE, &filter);
+        SHAMapHash{mLedger->info().txHash}, data, &filter);
     return san.isGood();
 }
 
@@ -1156,28 +1094,38 @@ InboundLedger::processData(
 
         SHAMapAddNode san;
 
-        if (!mHaveHeader)
+        try
         {
-            if (takeHeader(packet.nodes(0).nodedata()))
-                san.incUseful();
-            else
+            if (!mHaveHeader)
             {
-                JLOG(m_journal.warn()) << "Got invalid header data";
-                peer->charge(Resource::feeInvalidRequest);
-                return -1;
+                if (!takeHeader(packet.nodes(0).nodedata()))
+                {
+                    JLOG(m_journal.warn()) << "Got invalid header data";
+                    peer->charge(Resource::feeInvalidRequest);
+                    return -1;
+                }
+
+                san.incUseful();
+            }
+
+            if (!mHaveState && (packet.nodes().size() > 1) &&
+                !takeAsRootNode(makeSlice(packet.nodes(1).nodedata()), san))
+            {
+                JLOG(m_journal.warn()) << "Included AS root invalid";
+            }
+
+            if (!mHaveTransactions && (packet.nodes().size() > 2) &&
+                !takeTxRootNode(makeSlice(packet.nodes(2).nodedata()), san))
+            {
+                JLOG(m_journal.warn()) << "Included TX root invalid";
             }
         }
-
-        if (!mHaveState && (packet.nodes().size() > 1) &&
-            !takeAsRootNode(makeSlice(packet.nodes(1).nodedata()), san))
+        catch (std::exception const& ex)
         {
-            JLOG(m_journal.warn()) << "Included AS root invalid";
-        }
-
-        if (!mHaveTransactions && (packet.nodes().size() > 2) &&
-            !takeTxRootNode(makeSlice(packet.nodes(2).nodedata()), san))
-        {
-            JLOG(m_journal.warn()) << "Included TX root invalid";
+            JLOG(m_journal.warn())
+                << "Included AS/TX root invalid: " << ex.what();
+            peer->charge(Resource::feeBadData);
+            return -1;
         }
 
         if (san.isUseful())
@@ -1197,38 +1145,26 @@ InboundLedger::processData(
             return -1;
         }
 
-        std::vector<SHAMapNodeID> nodeIDs;
-        nodeIDs.reserve(packet.nodes().size());
-        std::vector<Blob> nodeData;
-        nodeData.reserve(packet.nodes().size());
-
-        for (int i = 0; i < packet.nodes().size(); ++i)
+        // Verify node IDs and data are complete
+        for (auto const& node : packet.nodes())
         {
-            const protocol::TMLedgerNode& node = packet.nodes(i);
-
             if (!node.has_nodeid() || !node.has_nodedata())
             {
                 JLOG(m_journal.warn()) << "Got bad node";
                 peer->charge(Resource::feeInvalidRequest);
                 return -1;
             }
-
-            nodeIDs.push_back(
-                SHAMapNodeID(node.nodeid().data(), node.nodeid().size()));
-            nodeData.push_back(
-                Blob(node.nodedata().begin(), node.nodedata().end()));
         }
 
         SHAMapAddNode san;
+        receiveNode(packet, san);
 
         if (packet.type() == protocol::liTX_NODE)
         {
-            takeTxNode(nodeIDs, nodeData, san);
             JLOG(m_journal.debug()) << "Ledger TX node stats: " << san.get();
         }
         else
         {
-            takeAsNode(nodeIDs, nodeData, san);
             JLOG(m_journal.debug()) << "Ledger AS node stats: " << san.get();
         }
 

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

@@ -247,15 +247,8 @@ public:
                 if (!node.has_nodeid() || !node.has_nodedata())
                     return;
 
-                auto id_string = node.nodeid();
-                auto newNode = SHAMapAbstractNode::make(
-                    makeSlice(node.nodedata()),
-                    0,
-                    snfWIRE,
-                    SHAMapHash{uZero},
-                    false,
-                    app_.journal("SHAMapNodeID"),
-                    SHAMapNodeID(id_string.data(), id_string.size()));
+                auto newNode = SHAMapAbstractNode::makeFromWire(
+                    makeSlice(node.nodedata()));
 
                 if (!newNode)
                     return;
@@ -263,10 +256,9 @@ public:
                 s.erase();
                 newNode->addRaw(s, snfPREFIX);
 
-                auto blob = std::make_shared<Blob>(s.begin(), s.end());
-
                 app_.getLedgerMaster().addFetchPack(
-                    newNode->getNodeHash().as_uint256(), blob);
+                    newNode->getNodeHash().as_uint256(),
+                    std::make_shared<Blob>(s.begin(), s.end()));
             }
         }
         catch (std::exception const&)

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

@@ -1303,41 +1303,6 @@ LedgerMaster::tryAdvance()
     }
 }
 
-// Return the hash of the valid ledger with a particular sequence, given a
-// subsequent ledger known valid.
-boost::optional<LedgerHash>
-LedgerMaster::getLedgerHash(
-    std::uint32_t desiredSeq,
-    std::shared_ptr<ReadView const> const& knownGoodLedger)
-{
-    assert(desiredSeq < knownGoodLedger->info().seq);
-
-    auto hash = hashOfSeq(*knownGoodLedger, desiredSeq, m_journal);
-
-    // Not directly in the given ledger
-    if (!hash)
-    {
-        std::uint32_t seq = (desiredSeq + 255) % 256;
-        assert(seq < desiredSeq);
-
-        hash = hashOfSeq(*knownGoodLedger, seq, m_journal);
-        if (hash)
-        {
-            if (auto l = getLedgerByHash(*hash))
-            {
-                hash = hashOfSeq(*l, desiredSeq, m_journal);
-                assert(hash);
-            }
-        }
-        else
-        {
-            assert(false);
-        }
-    }
-
-    return hash;
-}
-
 void
 LedgerMaster::updatePaths(Job& job)
 {
@@ -1948,7 +1913,7 @@ LedgerMaster::doAdvance(std::unique_lock<std::recursive_mutex>& sl)
 }
 
 void
-LedgerMaster::addFetchPack(uint256 const& hash, std::shared_ptr<Blob>& data)
+LedgerMaster::addFetchPack(uint256 const& hash, std::shared_ptr<Blob> data)
 {
     fetch_packs_.canonicalize_replace_client(hash, data);
 }

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

@@ -212,7 +212,6 @@ TransactionAcquire::takeNodes(
                 else if (!mMap->addRootNode(
                                   SHAMapHash{mHash},
                                   makeSlice(*nodeDatait),
-                                  snfWIRE,
                                   nullptr)
                               .isGood())
                 {

src/ripple/overlay/impl/PeerImp.cpp

@@ -1184,7 +1184,15 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetPeerShardInfo> const& m)
                 reply.set_lastlink(true);
 
             if (m->peerchain_size() > 0)
+            {
+                for (int i = 0; i < m->peerchain_size(); ++i)
+                {
+                    if (!publicKeyType(makeSlice(m->peerchain(i).nodepubkey())))
+                        return badData("Invalid peer chain public key");
+                }
+
                 *reply.mutable_peerchain() = m->peerchain();
+            }
 
             send(std::make_shared<Message>(reply, protocol::mtPEER_SHARD_INFO));
 
@@ -2209,7 +2217,15 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetObjectByHash> const& m)
         reply.set_type(packet.type());
 
         if (packet.has_ledgerhash())
+        {
+            if (!stringIsUint256Sized(packet.ledgerhash()))
+            {
+                fee_ = Resource::feeInvalidRequest;
+                return;
+            }
+
             reply.set_ledgerhash(packet.ledgerhash());
+        }
 
         // This is a very minimal implementation
         for (int i = 0; i < packet.objects_size(); ++i)
@@ -2290,10 +2306,10 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMGetObjectByHash> const& m)
                 {
                     uint256 const hash{obj.hash()};
 
-                    std::shared_ptr<Blob> data(std::make_shared<Blob>(
-                        obj.data().begin(), obj.data().end()));
-
-                    app_.getLedgerMaster().addFetchPack(hash, data);
+                    app_.getLedgerMaster().addFetchPack(
+                        hash,
+                        std::make_shared<Blob>(
+                            obj.data().begin(), obj.data().end()));
                 }
             }
         }
@@ -2587,16 +2603,15 @@ PeerImp::getLedger(std::shared_ptr<protocol::TMGetLedger> const& m)
             {
                 JLOG(p_journal_.debug()) << "GetLedger: Routing Tx set request";
 
-                auto const v = getPeerWithTree(overlay_, txHash, this);
-                if (!v)
+                if (auto const v = getPeerWithTree(overlay_, txHash, this))
                 {
-                    JLOG(p_journal_.info()) << "GetLedger: Route TX set failed";
+                    packet.set_requestcookie(id());
+                    v->send(std::make_shared<Message>(
+                        packet, protocol::mtGET_LEDGER));
                     return;
                 }
 
-                packet.set_requestcookie(id());
-                v->send(
-                    std::make_shared<Message>(packet, protocol::mtGET_LEDGER));
+                JLOG(p_journal_.info()) << "GetLedger: Route TX set failed";
                 return;
             }
 

src/ripple/shamap/SHAMap.h

@@ -246,7 +246,6 @@ public:
     addRootNode(
         SHAMapHash const& hash,
         Slice const& rootNode,
-        SHANodeFormat format,
         SHAMapSyncFilter* filter);
     SHAMapAddNode
     addKnownNode(

src/ripple/shamap/SHAMapTreeNode.h

@@ -176,14 +176,32 @@ public:
     invariants(bool is_root = false) const = 0;
 
     static std::shared_ptr<SHAMapAbstractNode>
-    make(
-        Slice const& rawNode,
+    makeFromPrefix(Slice rawNode, SHAMapHash const& hash);
+
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeFromWire(Slice rawNode);
+
+private:
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeTransaction(
+        Slice data,
         std::uint32_t seq,
-        SHANodeFormat format,
         SHAMapHash const& hash,
-        bool hashValid,
-        beast::Journal j,
-        SHAMapNodeID const& id = SHAMapNodeID{});
+        bool hashValid);
+
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeAccountState(
+        Slice data,
+        std::uint32_t seq,
+        SHAMapHash const& hash,
+        bool hashValid);
+
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeTransactionWithMeta(
+        Slice data,
+        std::uint32_t seq,
+        SHAMapHash const& hash,
+        bool hashValid);
 };
 
 class SHAMapInnerNode : public SHAMapAbstractNode
@@ -239,15 +257,15 @@ public:
     void
     invariants(bool is_root = false) const override;
 
-    friend std::shared_ptr<SHAMapAbstractNode>
-    SHAMapAbstractNode::make(
-        Slice const& rawNode,
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeFullInner(
+        Slice data,
         std::uint32_t seq,
-        SHANodeFormat format,
         SHAMapHash const& hash,
-        bool hashValid,
-        beast::Journal j,
-        SHAMapNodeID const& id);
+        bool hashValid);
+
+    static std::shared_ptr<SHAMapAbstractNode>
+    makeCompressedInner(Slice data, std::uint32_t seq);
 };
 
 // SHAMapTreeNode represents a leaf, and may eventually be renamed to reflect
@@ -282,10 +300,6 @@ public:
     invariants(bool is_root = false) const override;
 
 public:  // public only to SHAMap
-    // inner node functions
-    bool
-    isInnerNode() const;
-
     // item node function
     bool
     hasItem() const;
@@ -399,12 +413,6 @@ SHAMapInnerNode::setFullBelowGen(std::uint32_t gen)
 
 // SHAMapTreeNode
 
-inline bool
-SHAMapTreeNode::isInnerNode() const
-{
-    return !mItem;
-}
-
 inline bool
 SHAMapTreeNode::hasItem() const
 {

src/ripple/shamap/impl/SHAMap.cpp

@@ -151,13 +151,8 @@ SHAMap::fetchNodeFromDB(SHAMapHash const& hash) const
         {
             try
             {
-                node = SHAMapAbstractNode::make(
-                    makeSlice(obj->getData()),
-                    0,
-                    snfPREFIX,
-                    hash,
-                    true,
-                    f_.journal());
+                node = SHAMapAbstractNode::makeFromPrefix(
+                    makeSlice(obj->getData()), hash);
                 if (node)
                     canonicalize(hash, node);
             }
@@ -181,20 +176,32 @@ SHAMap::fetchNodeFromDB(SHAMapHash const& hash) const
 std::shared_ptr<SHAMapAbstractNode>
 SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter* filter) const
 {
-    std::shared_ptr<SHAMapAbstractNode> node;
     if (auto nodeData = filter->getNode(hash))
     {
-        node = SHAMapAbstractNode::make(
-            makeSlice(*nodeData), 0, snfPREFIX, hash, true, f_.journal());
-        if (node)
+        try
         {
-            filter->gotNode(
-                true, hash, ledgerSeq_, std::move(*nodeData), node->getType());
-            if (backed_)
-                canonicalize(hash, node);
+            auto node =
+                SHAMapAbstractNode::makeFromPrefix(makeSlice(*nodeData), hash);
+            if (node)
+            {
+                filter->gotNode(
+                    true,
+                    hash,
+                    ledgerSeq_,
+                    std::move(*nodeData),
+                    node->getType());
+                if (backed_)
+                    canonicalize(hash, node);
+            }
+            return node;
+        }
+        catch (std::exception const& x)
+        {
+            JLOG(f_.journal().warn())
+                << "Invalid node/data, hash=" << hash << ": " << x.what();
         }
     }
-    return node;
+    return {};
 }
 
 // Get a node without throwing
@@ -374,13 +381,8 @@ SHAMap::descendAsync(
             if (!obj)
                 return nullptr;
 
-            ptr = SHAMapAbstractNode::make(
-                makeSlice(obj->getData()),
-                0,
-                snfPREFIX,
-                hash,
-                true,
-                f_.journal());
+            ptr = SHAMapAbstractNode::makeFromPrefix(
+                makeSlice(obj->getData()), hash);
             if (ptr && backed_)
                 canonicalize(hash, ptr);
         }

src/ripple/shamap/impl/SHAMapSync.cpp

@@ -546,7 +546,6 @@ SHAMapAddNode
 SHAMap::addRootNode(
     SHAMapHash const& hash,
     Slice const& rootNode,
-    SHANodeFormat format,
     SHAMapSyncFilter* filter)
 {
     // we already have a root_ node
@@ -558,8 +557,7 @@ SHAMap::addRootNode(
     }
 
     assert(seq_ >= 1);
-    auto node = SHAMapAbstractNode::make(
-        rootNode, 0, format, SHAMapHash{}, false, f_.journal());
+    auto node = SHAMapAbstractNode::makeFromWire(rootNode);
     if (!node || !node->isValid() || node->getNodeHash() != hash)
         return SHAMapAddNode::invalid();
 
@@ -602,8 +600,7 @@ SHAMap::addKnownNode(
     }
 
     std::uint32_t generation = f_.fullbelow().getGeneration();
-    auto newNode = SHAMapAbstractNode::make(
-        rawNode, 0, snfWIRE, SHAMapHash{}, false, f_.journal(), node);
+    auto newNode = SHAMapAbstractNode::makeFromWire(rawNode);
     SHAMapNodeID iNodeID;
     auto iNode = root_.get();
 

src/ripple/shamap/impl/SHAMapTreeNode.cpp

@@ -76,220 +76,219 @@ SHAMapTreeNode::SHAMapTreeNode(
 }
 
 std::shared_ptr<SHAMapAbstractNode>
-SHAMapAbstractNode::make(
-    Slice const& rawNode,
+SHAMapAbstractNode::makeTransaction(
+    Slice data,
     std::uint32_t seq,
-    SHANodeFormat format,
     SHAMapHash const& hash,
-    bool hashValid,
-    beast::Journal j,
-    SHAMapNodeID const& id)
+    bool hashValid)
 {
-    if (format == snfWIRE)
+    // FIXME: using a Serializer results in a copy; avoid it?
+    Serializer s(data.begin(), data.size());
+
+    auto item = std::make_shared<SHAMapItem const>(
+        sha512Half(HashPrefix::transactionID, data), s);
+
+    if (hashValid)
+        return std::make_shared<SHAMapTreeNode>(
+            std::move(item), tnTRANSACTION_NM, seq, hash);
+
+    return std::make_shared<SHAMapTreeNode>(
+        std::move(item), tnTRANSACTION_NM, seq);
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapAbstractNode::makeTransactionWithMeta(
+    Slice data,
+    std::uint32_t seq,
+    SHAMapHash const& hash,
+    bool hashValid)
+{
+    Serializer s(data.data(), data.size());
+
+    uint256 tag;
+
+    if (s.size() < tag.bytes)
+        Throw<std::runtime_error>("Short TXN+MD node");
+
+    // FIXME: improve this interface so that the above check isn't needed
+    if (!s.getBitString(tag, s.size() - tag.bytes))
+        Throw<std::out_of_range>(
+            "Short TXN+MD node (" + std::to_string(s.size()) + ")");
+
+    s.chop(tag.bytes);
+
+    auto item = std::make_shared<SHAMapItem const>(tag, s.peekData());
+
+    if (hashValid)
+        return std::make_shared<SHAMapTreeNode>(
+            std::move(item), tnTRANSACTION_MD, seq, hash);
+
+    return std::make_shared<SHAMapTreeNode>(
+        std::move(item), tnTRANSACTION_MD, seq);
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapAbstractNode::makeAccountState(
+    Slice data,
+    std::uint32_t seq,
+    SHAMapHash const& hash,
+    bool hashValid)
+{
+    Serializer s(data.data(), data.size());
+
+    uint256 tag;
+
+    if (s.size() < tag.bytes)
+        Throw<std::runtime_error>("short AS node");
+
+    // FIXME: improve this interface so that the above check isn't needed
+    if (!s.getBitString(tag, s.size() - tag.bytes))
+        Throw<std::out_of_range>(
+            "Short AS node (" + std::to_string(s.size()) + ")");
+
+    s.chop(tag.bytes);
+
+    if (tag.isZero())
+        Throw<std::runtime_error>("Invalid AS node");
+
+    auto item = std::make_shared<SHAMapItem const>(tag, s.peekData());
+
+    if (hashValid)
+        return std::make_shared<SHAMapTreeNode>(
+            std::move(item), tnACCOUNT_STATE, seq, hash);
+
+    return std::make_shared<SHAMapTreeNode>(
+        std::move(item), tnACCOUNT_STATE, seq);
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapInnerNode::makeFullInner(
+    Slice data,
+    std::uint32_t seq,
+    SHAMapHash const& hash,
+    bool hashValid)
+{
+    if (data.size() != 512)
+        Throw<std::runtime_error>("Invalid FI node");
+
+    auto ret = std::make_shared<SHAMapInnerNode>(seq);
+
+    Serializer s(data.data(), data.size());
+
+    for (int i = 0; i < 16; ++i)
     {
-        if (rawNode.empty())
-            return {};
+        s.getBitString(ret->mHashes[i].as_uint256(), i * 32);
 
-        Serializer s(rawNode.data(), rawNode.size() - 1);
-        int type = rawNode[rawNode.size() - 1];
-        int len = s.getLength();
-
-        if ((type < 0) || (type > 6))
-            return {};
-        if (type == 0)
-        {
-            // transaction
-            auto item = std::make_shared<SHAMapItem const>(
-                sha512Half(
-                    HashPrefix::transactionID, Slice(s.data(), s.size())),
-                s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnTRANSACTION_NM, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnTRANSACTION_NM, seq);
-        }
-        else if (type == 1)
-        {
-            // account state
-            if (len < (256 / 8))
-                Throw<std::runtime_error>("short AS node");
-
-            uint256 u;
-            s.getBitString(u, len - (256 / 8));
-            s.chop(256 / 8);
-
-            if (u.isZero())
-                Throw<std::runtime_error>("invalid AS node");
-
-            auto item = std::make_shared<SHAMapItem const>(u, s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnACCOUNT_STATE, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnACCOUNT_STATE, seq);
-        }
-        else if (type == 2)
-        {
-            // full inner
-            if (len != 512)
-                Throw<std::runtime_error>("invalid FI node");
-
-            auto ret = std::make_shared<SHAMapInnerNode>(seq);
-            for (int i = 0; i < 16; ++i)
-            {
-                s.getBitString(ret->mHashes[i].as_uint256(), i * 32);
-
-                if (ret->mHashes[i].isNonZero())
-                    ret->mIsBranch |= (1 << i);
-            }
-            if (hashValid)
-                ret->mHash = hash;
-            else
-                ret->updateHash();
-            return ret;
-        }
-        else if (type == 3)
-        {
-            auto ret = std::make_shared<SHAMapInnerNode>(seq);
-            // compressed inner
-            for (int i = 0; i < (len / 33); ++i)
-            {
-                int pos;
-                if (!s.get8(pos, 32 + (i * 33)))
-                    Throw<std::runtime_error>("short CI node");
-                if ((pos < 0) || (pos >= 16))
-                    Throw<std::runtime_error>("invalid CI node");
-                s.getBitString(ret->mHashes[pos].as_uint256(), i * 33);
-                if (ret->mHashes[pos].isNonZero())
-                    ret->mIsBranch |= (1 << pos);
-            }
-            if (hashValid)
-                ret->mHash = hash;
-            else
-                ret->updateHash();
-            return ret;
-        }
-        else if (type == 4)
-        {
-            // transaction with metadata
-            if (len < (256 / 8))
-                Throw<std::runtime_error>("short TM node");
-
-            uint256 u;
-            s.getBitString(u, len - (256 / 8));
-            s.chop(256 / 8);
-
-            if (u.isZero())
-                Throw<std::runtime_error>("invalid TM node");
-
-            auto item = std::make_shared<SHAMapItem const>(u, s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnTRANSACTION_MD, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnTRANSACTION_MD, seq);
-        }
+        if (ret->mHashes[i].isNonZero())
+            ret->mIsBranch |= (1 << i);
     }
 
-    else if (format == snfPREFIX)
+    if (hashValid)
+        ret->mHash = hash;
+    else
+        ret->updateHash();
+    return ret;
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapInnerNode::makeCompressedInner(Slice data, std::uint32_t seq)
+{
+    Serializer s(data.data(), data.size());
+
+    int len = s.getLength();
+
+    auto ret = std::make_shared<SHAMapInnerNode>(seq);
+
+    for (int i = 0; i < (len / 33); ++i)
     {
-        if (rawNode.size() < 4)
-        {
-            JLOG(j.info()) << "size < 4";
-            Throw<std::runtime_error>("invalid P node");
-        }
+        int pos;
 
-        std::uint32_t prefix = rawNode[0];
-        prefix <<= 8;
-        prefix |= rawNode[1];
-        prefix <<= 8;
-        prefix |= rawNode[2];
-        prefix <<= 8;
-        prefix |= rawNode[3];
-        Serializer s(rawNode.data() + 4, rawNode.size() - 4);
+        if (!s.get8(pos, 32 + (i * 33)))
+            Throw<std::runtime_error>("short CI node");
 
-        if (safe_cast<HashPrefix>(prefix) == HashPrefix::transactionID)
-        {
-            auto item = std::make_shared<SHAMapItem const>(
-                sha512Half(rawNode), s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnTRANSACTION_NM, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnTRANSACTION_NM, seq);
-        }
-        else if (safe_cast<HashPrefix>(prefix) == HashPrefix::leafNode)
-        {
-            if (s.getLength() < 32)
-                Throw<std::runtime_error>("short PLN node");
+        if ((pos < 0) || (pos >= 16))
+            Throw<std::runtime_error>("invalid CI node");
 
-            uint256 u;
-            s.getBitString(u, s.getLength() - 32);
-            s.chop(32);
+        s.getBitString(ret->mHashes[pos].as_uint256(), i * 33);
 
-            if (u.isZero())
-            {
-                JLOG(j.info()) << "invalid PLN node";
-                Throw<std::runtime_error>("invalid PLN node");
-            }
-
-            auto item = std::make_shared<SHAMapItem const>(u, s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnACCOUNT_STATE, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnACCOUNT_STATE, seq);
-        }
-        else if (safe_cast<HashPrefix>(prefix) == HashPrefix::innerNode)
-        {
-            auto len = s.getLength();
-
-            if (len != 512)
-                Throw<std::runtime_error>("invalid PIN node");
-
-            auto ret = std::make_shared<SHAMapInnerNode>(seq);
-
-            for (int i = 0; i < 16; ++i)
-            {
-                s.getBitString(ret->mHashes[i].as_uint256(), i * 32);
-
-                if (ret->mHashes[i].isNonZero())
-                    ret->mIsBranch |= (1 << i);
-            }
-
-            if (hashValid)
-                ret->mHash = hash;
-            else
-                ret->updateHash();
-            return ret;
-        }
-        else if (safe_cast<HashPrefix>(prefix) == HashPrefix::txNode)
-        {
-            // transaction with metadata
-            if (s.getLength() < 32)
-                Throw<std::runtime_error>("short TXN node");
-
-            uint256 txID;
-            s.getBitString(txID, s.getLength() - 32);
-            s.chop(32);
-            auto item = std::make_shared<SHAMapItem const>(txID, s.peekData());
-            if (hashValid)
-                return std::make_shared<SHAMapTreeNode>(
-                    std::move(item), tnTRANSACTION_MD, seq, hash);
-            return std::make_shared<SHAMapTreeNode>(
-                std::move(item), tnTRANSACTION_MD, seq);
-        }
-        else
-        {
-            JLOG(j.info()) << "Unknown node prefix " << std::hex << prefix
-                           << std::dec;
-            Throw<std::runtime_error>("invalid node prefix");
-        }
+        if (ret->mHashes[pos].isNonZero())
+            ret->mIsBranch |= (1 << pos);
     }
-    assert(false);
-    Throw<std::runtime_error>("Unknown format");
-    return {};  // Silence compiler warning.
+
+    ret->updateHash();
+
+    return ret;
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapAbstractNode::makeFromWire(Slice rawNode)
+{
+    if (rawNode.empty())
+        return {};
+
+    auto const type = rawNode[rawNode.size() - 1];
+
+    rawNode.remove_suffix(1);
+
+    bool const hashValid = false;
+    SHAMapHash const hash;
+
+    std::uint32_t const seq = 0;
+
+    if (type == 0)
+        return makeTransaction(rawNode, seq, hash, hashValid);
+
+    if (type == 1)
+        return makeAccountState(rawNode, seq, hash, hashValid);
+
+    if (type == 2)
+        return SHAMapInnerNode::makeFullInner(rawNode, seq, hash, hashValid);
+
+    if (type == 3)
+        return SHAMapInnerNode::makeCompressedInner(rawNode, seq);
+
+    if (type == 4)
+        return makeTransactionWithMeta(rawNode, seq, hash, hashValid);
+
+    Throw<std::runtime_error>(
+        "wire: Unknown type (" + std::to_string(type) + ")");
+}
+
+std::shared_ptr<SHAMapAbstractNode>
+SHAMapAbstractNode::makeFromPrefix(Slice rawNode, SHAMapHash const& hash)
+{
+    if (rawNode.size() < 4)
+        Throw<std::runtime_error>("prefix: short node");
+
+    // FIXME: Use SerialIter::get32?
+    // Extract the prefix
+    auto const type = safe_cast<HashPrefix>(
+        (safe_cast<std::uint32_t>(rawNode[0]) << 24) +
+        (safe_cast<std::uint32_t>(rawNode[1]) << 16) +
+        (safe_cast<std::uint32_t>(rawNode[2]) << 8) +
+        (safe_cast<std::uint32_t>(rawNode[3])));
+
+    rawNode.remove_prefix(4);
+
+    bool const hashValid = true;
+    std::uint32_t const seq = 0;
+
+    if (type == HashPrefix::transactionID)
+        return makeTransaction(rawNode, seq, hash, hashValid);
+
+    if (type == HashPrefix::leafNode)
+        return makeAccountState(rawNode, seq, hash, hashValid);
+
+    if (type == HashPrefix::innerNode)
+        return SHAMapInnerNode::makeFullInner(rawNode, seq, hash, hashValid);
+
+    if (type == HashPrefix::txNode)
+        return makeTransactionWithMeta(rawNode, seq, hash, hashValid);
+
+    Throw<std::runtime_error>(
+        "prefix: unknown type (" +
+        std::to_string(safe_cast<std::underlying_type_t<HashPrefix>>(type)) +
+        ")");
 }
 
 bool

src/test/shamap/SHAMapSync_test.cpp

@@ -140,7 +140,6 @@ public:
                              .addRootNode(
                                  source.getHash(),
                                  makeSlice(*gotNodes_a.begin()),
-                                 snfWIRE,
                                  nullptr)
                              .isGood());
         }