Avoid using std::shared_ptr when not necessary: (#4218)

The `Ledger` class contains two `SHAMap` instances: the state and transaction maps. Previously, the maps were dynamically allocated using `std::make_shared` despite the fact that they did not require lifetime management separate from the lifetime of the `Ledger` instance to which they belong. The two `SHAMap` instances are now regular member variables. Some smart pointers and dynamic memory allocation was avoided by using stack-based alternatives. Commit 3 of 3 in #4218.
2025-12-06 17:27:55 +00:00 · 2023-04-11 15:50:25 -07:00
parent c3acbce82d
commit 066f91ca07
10 changed files with 144 additions and 148 deletions
--- a/src/ripple/app/ledger/Ledger.cpp
+++ b/src/ripple/app/ledger/Ledger.cpp
@@ -182,8 +182,8 @@ Ledger::Ledger(
    std::vector<uint256> const& amendments,
    Family& family)
    : mImmutable(false)
-    , txMap_(std::make_shared<SHAMap>(SHAMapType::TRANSACTION, family))
-    , stateMap_(std::make_shared<SHAMap>(SHAMapType::STATE, family))
+    , txMap_(SHAMapType::TRANSACTION, family)
+    , stateMap_(SHAMapType::STATE, family)
    , rules_{config.features}
    , j_(beast::Journal(beast::Journal::getNullSink()))
 {
@@ -235,7 +235,7 @@ Ledger::Ledger(
        rawInsert(sle);
    }

-    stateMap_->flushDirty(hotACCOUNT_NODE);
+    stateMap_.flushDirty(hotACCOUNT_NODE);
    setImmutable();
 }

@@ -247,12 +247,8 @@ Ledger::Ledger(
    Family& family,
    beast::Journal j)
    : mImmutable(true)
-    , txMap_(std::make_shared<SHAMap>(
-          SHAMapType::TRANSACTION,
-          info.txHash,
-          family))
-    , stateMap_(
-          std::make_shared<SHAMap>(SHAMapType::STATE, info.accountHash, family))
+    , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
+    , stateMap_(SHAMapType::STATE, info.accountHash, family)
    , rules_(config.features)
    , info_(info)
    , j_(j)
@@ -260,7 +256,7 @@ Ledger::Ledger(
    loaded = true;

    if (info_.txHash.isNonZero() &&
-        !txMap_->fetchRoot(SHAMapHash{info_.txHash}, nullptr))
+        !txMap_.fetchRoot(SHAMapHash{info_.txHash}, nullptr))
    {
        if (config.reporting())
        {
@@ -272,7 +268,7 @@ Ledger::Ledger(
    }

    if (info_.accountHash.isNonZero() &&
-        !stateMap_->fetchRoot(SHAMapHash{info_.accountHash}, nullptr))
+        !stateMap_.fetchRoot(SHAMapHash{info_.accountHash}, nullptr))
    {
        if (config.reporting())
        {
@@ -283,8 +279,8 @@ Ledger::Ledger(
        JLOG(j.warn()) << "Don't have state data root for ledger" << info_.seq;
    }

-    txMap_->setImmutable();
-    stateMap_->setImmutable();
+    txMap_.setImmutable();
+    stateMap_.setImmutable();

    defaultFees(config);
    if (!setup())
@@ -301,10 +297,8 @@ Ledger::Ledger(
 // Create a new ledger that follows this one
 Ledger::Ledger(Ledger const& prevLedger, NetClock::time_point closeTime)
    : mImmutable(false)
-    , txMap_(std::make_shared<SHAMap>(
-          SHAMapType::TRANSACTION,
-          prevLedger.stateMap_->family()))
-    , stateMap_(prevLedger.stateMap_->snapShot(true))
+    , txMap_(SHAMapType::TRANSACTION, prevLedger.txMap_.family())
+    , stateMap_(prevLedger.stateMap_, true)
    , fees_(prevLedger.fees_)
    , rules_(prevLedger.rules_)
    , j_(beast::Journal(beast::Journal::getNullSink()))
@@ -333,12 +327,8 @@ Ledger::Ledger(Ledger const& prevLedger, NetClock::time_point closeTime)

 Ledger::Ledger(LedgerInfo const& info, Config const& config, Family& family)
    : mImmutable(true)
-    , txMap_(std::make_shared<SHAMap>(
-          SHAMapType::TRANSACTION,
-          info.txHash,
-          family))
-    , stateMap_(
-          std::make_shared<SHAMap>(SHAMapType::STATE, info.accountHash, family))
+    , txMap_(SHAMapType::TRANSACTION, info.txHash, family)
+    , stateMap_(SHAMapType::STATE, info.accountHash, family)
    , rules_{config.features}
    , info_(info)
    , j_(beast::Journal(beast::Journal::getNullSink()))
@@ -352,8 +342,8 @@ Ledger::Ledger(
    Config const& config,
    Family& family)
    : mImmutable(false)
-    , txMap_(std::make_shared<SHAMap>(SHAMapType::TRANSACTION, family))
-    , stateMap_(std::make_shared<SHAMap>(SHAMapType::STATE, family))
+    , txMap_(SHAMapType::TRANSACTION, family)
+    , stateMap_(SHAMapType::STATE, family)
    , rules_{config.features}
    , j_(beast::Journal(beast::Journal::getNullSink()))
 {
@@ -371,16 +361,16 @@ Ledger::setImmutable(bool rehash)
    // place the hash transitions to valid
    if (!mImmutable && rehash)
    {
-        info_.txHash = txMap_->getHash().as_uint256();
-        info_.accountHash = stateMap_->getHash().as_uint256();
+        info_.txHash = txMap_.getHash().as_uint256();
+        info_.accountHash = stateMap_.getHash().as_uint256();
    }

    if (rehash)
        info_.hash = calculateLedgerHash(info_);

    mImmutable = true;
-    txMap_->setImmutable();
-    stateMap_->setImmutable();
+    txMap_.setImmutable();
+    stateMap_.setImmutable();
    setup();
 }

@@ -403,7 +393,7 @@ bool
 Ledger::addSLE(SLE const& sle)
 {
    auto const s = sle.getSerializer();
-    return stateMap_->addItem(
+    return stateMap_.addItem(
        SHAMapNodeType::tnACCOUNT_STATE, make_shamapitem(sle.key(), s.slice()));
 }

@@ -439,20 +429,20 @@ bool
 Ledger::exists(Keylet const& k) const
 {
    // VFALCO NOTE Perhaps check the type for debug builds?
-    return stateMap_->hasItem(k.key);
+    return stateMap_.hasItem(k.key);
 }

 bool
 Ledger::exists(uint256 const& key) const
 {
-    return stateMap_->hasItem(key);
+    return stateMap_.hasItem(key);
 }

 std::optional<uint256>
 Ledger::succ(uint256 const& key, std::optional<uint256> const& last) const
 {
-    auto item = stateMap_->upper_bound(key);
-    if (item == stateMap_->end())
+    auto item = stateMap_.upper_bound(key);
+    if (item == stateMap_.end())
        return std::nullopt;
    if (last && item->key() >= last)
        return std::nullopt;
@@ -467,7 +457,7 @@ Ledger::read(Keylet const& k) const
        assert(false);
        return nullptr;
    }
-    auto const& item = stateMap_->peekItem(k.key);
+    auto const& item = stateMap_.peekItem(k.key);
    if (!item)
        return nullptr;
    auto sle = std::make_shared<SLE>(SerialIter{item->slice()}, item->key());
@@ -481,45 +471,44 @@ Ledger::read(Keylet const& k) const
 auto
 Ledger::slesBegin() const -> std::unique_ptr<sles_type::iter_base>
 {
-    return std::make_unique<sles_iter_impl>(stateMap_->begin());
+    return std::make_unique<sles_iter_impl>(stateMap_.begin());
 }

 auto
 Ledger::slesEnd() const -> std::unique_ptr<sles_type::iter_base>
 {
-    return std::make_unique<sles_iter_impl>(stateMap_->end());
+    return std::make_unique<sles_iter_impl>(stateMap_.end());
 }

 auto
 Ledger::slesUpperBound(uint256 const& key) const
    -> std::unique_ptr<sles_type::iter_base>
 {
-    return std::make_unique<sles_iter_impl>(stateMap_->upper_bound(key));
+    return std::make_unique<sles_iter_impl>(stateMap_.upper_bound(key));
 }

 auto
 Ledger::txsBegin() const -> std::unique_ptr<txs_type::iter_base>
 {
-    return std::make_unique<txs_iter_impl>(!open(), txMap_->begin());
+    return std::make_unique<txs_iter_impl>(!open(), txMap_.begin());
 }

 auto
 Ledger::txsEnd() const -> std::unique_ptr<txs_type::iter_base>
 {
-    return std::make_unique<txs_iter_impl>(!open(), txMap_->end());
+    return std::make_unique<txs_iter_impl>(!open(), txMap_.end());
 }

 bool
 Ledger::txExists(uint256 const& key) const
 {
-    return txMap_->hasItem(key);
+    return txMap_.hasItem(key);
 }

 auto
 Ledger::txRead(key_type const& key) const -> tx_type
 {
-    assert(txMap_);
-    auto const& item = txMap_->peekItem(key);
+    auto const& item = txMap_.peekItem(key);
    if (!item)
        return {};
    if (!open())
@@ -536,7 +525,7 @@ Ledger::digest(key_type const& key) const -> std::optional<digest_type>
    SHAMapHash digest;
    // VFALCO Unfortunately this loads the item
    //        from the NodeStore needlessly.
-    if (!stateMap_->peekItem(key, digest))
+    if (!stateMap_.peekItem(key, digest))
        return std::nullopt;
    return digest.as_uint256();
 }
@@ -546,14 +535,14 @@ Ledger::digest(key_type const& key) const -> std::optional<digest_type>
 void
 Ledger::rawErase(std::shared_ptr<SLE> const& sle)
 {
-    if (!stateMap_->delItem(sle->key()))
+    if (!stateMap_.delItem(sle->key()))
        LogicError("Ledger::rawErase: key not found");
 }

 void
 Ledger::rawErase(uint256 const& key)
 {
-    if (!stateMap_->delItem(key))
+    if (!stateMap_.delItem(key))
        LogicError("Ledger::rawErase: key not found");
 }

@@ -562,7 +551,7 @@ Ledger::rawInsert(std::shared_ptr<SLE> const& sle)
 {
    Serializer ss;
    sle->add(ss);
-    if (!stateMap_->addGiveItem(
+    if (!stateMap_.addGiveItem(
            SHAMapNodeType::tnACCOUNT_STATE,
            make_shamapitem(sle->key(), ss.slice())))
        LogicError("Ledger::rawInsert: key already exists");
@@ -573,7 +562,7 @@ Ledger::rawReplace(std::shared_ptr<SLE> const& sle)
 {
    Serializer ss;
    sle->add(ss);
-    if (!stateMap_->updateGiveItem(
+    if (!stateMap_.updateGiveItem(
            SHAMapNodeType::tnACCOUNT_STATE,
            make_shamapitem(sle->key(), ss.slice())))
        LogicError("Ledger::rawReplace: key not found");
@@ -591,7 +580,7 @@ Ledger::rawTxInsert(
    Serializer s(txn->getDataLength() + metaData->getDataLength() + 16);
    s.addVL(txn->peekData());
    s.addVL(metaData->peekData());
-    if (!txMap().addGiveItem(
+    if (!txMap_.addGiveItem(
            SHAMapNodeType::tnTRANSACTION_MD, make_shamapitem(key, s.slice())))
        LogicError("duplicate_tx: " + to_string(key));
 }
@@ -610,7 +599,7 @@ Ledger::rawTxInsertWithHash(
    s.addVL(metaData->peekData());
    auto item = make_shamapitem(key, s.slice());
    auto hash = sha512Half(HashPrefix::txNode, item->slice(), item->key());
-    if (!txMap().addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, std::move(item)))
+    if (!txMap_.addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, std::move(item)))
        LogicError("duplicate_tx: " + to_string(key));

    return hash;
@@ -710,7 +699,7 @@ Ledger::defaultFees(Config const& config)
 std::shared_ptr<SLE>
 Ledger::peek(Keylet const& k) const
 {
-    auto const& value = stateMap_->peekItem(k.key);
+    auto const& value = stateMap_.peekItem(k.key);
    if (!value)
        return nullptr;
    auto sle = std::make_shared<SLE>(SerialIter{value->slice()}, value->key());
@@ -832,8 +821,8 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
    std::vector<SHAMapMissingNode> missingNodes1;
    std::vector<SHAMapMissingNode> missingNodes2;

-    if (stateMap_->getHash().isZero() && !info_.accountHash.isZero() &&
-        !stateMap_->fetchRoot(SHAMapHash{info_.accountHash}, nullptr))
+    if (stateMap_.getHash().isZero() && !info_.accountHash.isZero() &&
+        !stateMap_.fetchRoot(SHAMapHash{info_.accountHash}, nullptr))
    {
        missingNodes1.emplace_back(
            SHAMapType::STATE, SHAMapHash{info_.accountHash});
@@ -841,9 +830,9 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
    else
    {
        if (parallel)
-            return stateMap_->walkMapParallel(missingNodes1, 32);
+            return stateMap_.walkMapParallel(missingNodes1, 32);
        else
-            stateMap_->walkMap(missingNodes1, 32);
+            stateMap_.walkMap(missingNodes1, 32);
    }

    if (!missingNodes1.empty())
@@ -855,15 +844,15 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
        }
    }

-    if (txMap_->getHash().isZero() && info_.txHash.isNonZero() &&
-        !txMap_->fetchRoot(SHAMapHash{info_.txHash}, nullptr))
+    if (txMap_.getHash().isZero() && info_.txHash.isNonZero() &&
+        !txMap_.fetchRoot(SHAMapHash{info_.txHash}, nullptr))
    {
        missingNodes2.emplace_back(
            SHAMapType::TRANSACTION, SHAMapHash{info_.txHash});
    }
    else
    {
-        txMap_->walkMap(missingNodes2, 32);
+        txMap_.walkMap(missingNodes2, 32);
    }

    if (!missingNodes2.empty())
@@ -880,9 +869,9 @@ Ledger::walkLedger(beast::Journal j, bool parallel) const
 bool
 Ledger::assertSensible(beast::Journal ledgerJ) const
 {
-    if (info_.hash.isNonZero() && info_.accountHash.isNonZero() && stateMap_ &&
-        txMap_ && (info_.accountHash == stateMap_->getHash().as_uint256()) &&
-        (info_.txHash == txMap_->getHash().as_uint256()))
+    if (info_.hash.isNonZero() && info_.accountHash.isNonZero() &&
+        (info_.accountHash == stateMap_.getHash().as_uint256()) &&
+        (info_.txHash == txMap_.getHash().as_uint256()))
    {
        return true;
    }
@@ -1044,15 +1033,14 @@ pendSaveValidated(
        return true;
    }

-    JobType const jobType{isCurrent ? jtPUBLEDGER : jtPUBOLDLEDGER};
-    char const* const jobName{
-        isCurrent ? "Ledger::pendSave" : "Ledger::pendOldSave"};
-
    // See if we can use the JobQueue.
    if (!isSynchronous &&
-        app.getJobQueue().addJob(jobType, jobName, [&app, ledger, isCurrent]() {
-            saveValidatedLedger(app, ledger, isCurrent);
-        }))
+        app.getJobQueue().addJob(
+            isCurrent ? jtPUBLEDGER : jtPUBOLDLEDGER,
+            std::to_string(ledger->seq()),
+            [&app, ledger, isCurrent]() {
+                saveValidatedLedger(app, ledger, isCurrent);
+            }))
    {
        return true;
    }
@@ -1064,15 +1052,15 @@ pendSaveValidated(
 void
 Ledger::unshare() const
 {
-    stateMap_->unshare();
-    txMap_->unshare();
+    stateMap_.unshare();
+    txMap_.unshare();
 }

 void
 Ledger::invariants() const
 {
-    stateMap_->invariants();
-    txMap_->invariants();
+    stateMap_.invariants();
+    txMap_.invariants();
 }
 //------------------------------------------------------------------------------

--- a/src/ripple/app/ledger/Ledger.h
+++ b/src/ripple/app/ledger/Ledger.h
@@ -83,6 +83,10 @@ public:
    Ledger&
    operator=(Ledger const&) = delete;

+    Ledger(Ledger&&) = delete;
+    Ledger&
+    operator=(Ledger&&) = delete;
+
    /** Create the Genesis ledger.

        The Genesis ledger contains a single account whose
@@ -290,10 +294,10 @@ public:
    void
    setFull() const
    {
-        txMap_->setFull();
-        stateMap_->setFull();
-        txMap_->setLedgerSeq(info_.seq);
-        stateMap_->setLedgerSeq(info_.seq);
+        txMap_.setFull();
+        txMap_.setLedgerSeq(info_.seq);
+        stateMap_.setFull();
+        stateMap_.setLedgerSeq(info_.seq);
    }

    void
@@ -305,25 +309,25 @@ public:
    SHAMap const&
    stateMap() const
    {
-        return *stateMap_;
+        return stateMap_;
    }

    SHAMap&
    stateMap()
    {
-        return *stateMap_;
+        return stateMap_;
    }

    SHAMap const&
    txMap() const
    {
-        return *txMap_;
+        return txMap_;
    }

    SHAMap&
    txMap()
    {
-        return *txMap_;
+        return txMap_;
    }

    // returns false on error
@@ -401,8 +405,11 @@ private:

    bool mImmutable;

-    std::shared_ptr<SHAMap> txMap_;
-    std::shared_ptr<SHAMap> stateMap_;
+    // A SHAMap containing the transactions associated with this ledger.
+    SHAMap mutable txMap_;
+
+    // A SHAMap containing the state objects for this ledger.
+    SHAMap mutable stateMap_;

    // Protects fee variables
    std::mutex mutable mutex_;
--- a/src/ripple/app/ledger/LedgerHistory.cpp
+++ b/src/ripple/app/ledger/LedgerHistory.cpp
@@ -51,7 +51,9 @@ LedgerHistory::LedgerHistory(
 }

 bool
-LedgerHistory::insert(std::shared_ptr<Ledger const> ledger, bool validated)
+LedgerHistory::insert(
+    std::shared_ptr<Ledger const> const& ledger,
+    bool validated)
 {
    if (!ledger->isImmutable())
        LogicError("mutable Ledger in insert");
@@ -72,12 +74,9 @@ LedgerHash
 LedgerHistory::getLedgerHash(LedgerIndex index)
 {
    std::unique_lock sl(m_ledgers_by_hash.peekMutex());
-    auto it = mLedgersByIndex.find(index);
-
-    if (it != mLedgersByIndex.end())
+    if (auto it = mLedgersByIndex.find(index); it != mLedgersByIndex.end())
        return it->second;
-
-    return uint256();
+    return {};
 }

 std::shared_ptr<Ledger const>
@@ -167,19 +166,19 @@ log_metadata_difference(
    uint256 const& tx,
    beast::Journal j)
 {
-    auto getMeta = [](ReadView const& ledger,
-                      uint256 const& txID) -> std::shared_ptr<TxMeta> {
-        auto meta = ledger.txRead(txID).second;
-        if (!meta)
-            return {};
-        return std::make_shared<TxMeta>(txID, ledger.seq(), *meta);
+    auto getMeta = [](ReadView const& ledger, uint256 const& txID) {
+        std::optional<TxMeta> ret;
+        if (auto meta = ledger.txRead(txID).second)
+            ret.emplace(txID, ledger.seq(), *meta);
+        return ret;
    };

    auto validMetaData = getMeta(validLedger, tx);
    auto builtMetaData = getMeta(builtLedger, tx);
-    assert(validMetaData != nullptr || builtMetaData != nullptr);

-    if (validMetaData != nullptr && builtMetaData != nullptr)
+    assert(validMetaData || builtMetaData);
+
+    if (validMetaData && builtMetaData)
    {
        auto const& validNodes = validMetaData->getNodes();
        auto const& builtNodes = builtMetaData->getNodes();
@@ -280,17 +279,21 @@ log_metadata_difference(
                                << validNodes.getJson(JsonOptions::none);
            }
        }
+
+        return;
    }
-    else if (validMetaData != nullptr)
+
+    if (validMetaData)
    {
        JLOG(j.error()) << "MISMATCH on TX " << tx
-                        << ": Metadata Difference (built has none)\n"
+                        << ": Metadata Difference. Valid=\n"
                        << validMetaData->getJson(JsonOptions::none);
    }
-    else  // builtMetaData != nullptr
+
+    if (builtMetaData)
    {
        JLOG(j.error()) << "MISMATCH on TX " << tx
-                        << ": Metadata Difference (valid has none)\n"
+                        << ": Metadata Difference. Built=\n"
                        << builtMetaData->getJson(JsonOptions::none);
    }
 }
--- a/src/ripple/app/ledger/LedgerHistory.h
+++ b/src/ripple/app/ledger/LedgerHistory.h
@@ -44,7 +44,7 @@ public:
        @return `true` if the ledger was already tracked
    */
    bool
-    insert(std::shared_ptr<Ledger const> ledger, bool validated);
+    insert(std::shared_ptr<Ledger const> const& ledger, bool validated);

    /** Get the ledgers_by_hash cache hit rate
        @return the hit rate
--- a/src/ripple/app/ledger/impl/LedgerToJson.cpp
+++ b/src/ripple/app/ledger/impl/LedgerToJson.cpp
@@ -131,14 +131,14 @@ fillJsonTx(
        if (stMeta)
        {
            txJson[jss::metaData] = stMeta->getJson(JsonOptions::none);
+
+            // If applicable, insert delivered amount
            if (txnType == ttPAYMENT || txnType == ttCHECK_CASH)
-            {
-                // Insert delivered amount
-                auto txMeta = std::make_shared<TxMeta>(
-                    txn->getTransactionID(), fill.ledger.seq(), *stMeta);
                RPC::insertDeliveredAmount(
-                    txJson[jss::metaData], fill.ledger, txn, *txMeta);
-            }
+                    txJson[jss::metaData],
+                    fill.ledger,
+                    txn,
+                    {txn->getTransactionID(), fill.ledger.seq(), *stMeta});
        }
    }

--- a/src/ripple/app/rdb/impl/UnitaryShard.cpp
+++ b/src/ripple/app/rdb/impl/UnitaryShard.cpp
@@ -103,22 +103,24 @@ updateLedgerDBs(

            for (auto const& item : ledger->txs)
            {
-                if (stop)
+                if (stop.load(std::memory_order_relaxed))
                    return false;

-                auto const txID{item.first->getTransactionID()};
-                auto const sTxID{to_string(txID)};
-                auto const txMeta{std::make_shared<TxMeta>(
-                    txID, ledger->seq(), *item.second)};
+                TxMeta const txMeta{
+                    item.first->getTransactionID(),
+                    ledger->seq(),
+                    *item.second};
+
+                auto const sTxID = to_string(txMeta.getTxID());

                session << "DELETE FROM AccountTransactions "
                           "WHERE TransID = :txID;",
                    soci::use(sTxID);

-                auto const& accounts = txMeta->getAffectedAccounts();
+                auto const& accounts = txMeta.getAffectedAccounts();
                if (!accounts.empty())
                {
-                    auto const sTxnSeq{std::to_string(txMeta->getIndex())};
+                    auto const sTxnSeq{std::to_string(txMeta.getIndex())};
                    auto const s{boost::str(
                        boost::format("('%s','%s',%s,%s)") % sTxID % "%s" %
                        sSeq % sTxnSeq)};
--- a/src/ripple/nodestore/impl/DecodedBlob.cpp
+++ b/src/ripple/nodestore/impl/DecodedBlob.cpp
@@ -38,7 +38,6 @@ DecodedBlob::DecodedBlob(void const* key, void const* value, int valueBytes)

    m_success = false;
    m_key = key;
-    // VFALCO NOTE Ledger indexes should hav    e started at 1
    m_objectType = hotUNKNOWN;
    m_objectData = nullptr;
    m_dataBytes = std::max(0, valueBytes - 9);
--- a/src/ripple/protocol/impl/TxMeta.cpp
+++ b/src/ripple/protocol/impl/TxMeta.cpp
@@ -126,8 +126,7 @@ TxMeta::getAffectedAccounts() const

        if (index != -1)
        {
-            const STObject* inner =
-                dynamic_cast<const STObject*>(&it.peekAtIndex(index));
+            auto inner = dynamic_cast<STObject const*>(&it.peekAtIndex(index));
            assert(inner);
            if (inner)
            {
@@ -145,8 +144,7 @@ TxMeta::getAffectedAccounts() const
                        (field.getFName() == sfTakerPays) ||
                        (field.getFName() == sfTakerGets))
                    {
-                        const STAmount* lim =
-                            dynamic_cast<const STAmount*>(&field);
+                        auto lim = dynamic_cast<STAmount const*>(&field);
                        assert(lim);

                        if (lim != nullptr)
--- a/src/ripple/shamap/SHAMap.h
+++ b/src/ripple/shamap/SHAMap.h
@@ -124,10 +124,14 @@ public:
        boost::intrusive_ptr<SHAMapItem const>>;
    using Delta = std::map<uint256, DeltaItem>;

+    SHAMap() = delete;
    SHAMap(SHAMap const&) = delete;
    SHAMap&
    operator=(SHAMap const&) = delete;

+    // Take a snapshot of the given map:
+    SHAMap(SHAMap const& other, bool isMutable);
+
    // build new map
    SHAMap(SHAMapType t, Family& f);

--- a/src/ripple/shamap/impl/SHAMap.cpp
+++ b/src/ripple/shamap/impl/SHAMap.cpp
@@ -66,28 +66,28 @@ SHAMap::SHAMap(SHAMapType t, uint256 const& hash, Family& f)
    root_ = std::make_shared<SHAMapInnerNode>(cowid_);
 }

+SHAMap::SHAMap(SHAMap const& other, bool isMutable)
+    : f_(other.f_)
+    , journal_(other.f_.journal())
+    , cowid_(other.cowid_ + 1)
+    , ledgerSeq_(other.ledgerSeq_)
+    , root_(other.root_)
+    , state_(isMutable ? SHAMapState::Modifying : SHAMapState::Immutable)
+    , type_(other.type_)
+    , backed_(other.backed_)
+{
+    // If either map may change, they cannot share nodes
+    if ((state_ != SHAMapState::Immutable) ||
+        (other.state_ != SHAMapState::Immutable))
+    {
+        unshare();
+    }
+}
+
 std::shared_ptr<SHAMap>
 SHAMap::snapShot(bool isMutable) const
 {
-    auto ret = std::make_shared<SHAMap>(type_, f_);
-    SHAMap& newMap = *ret;
-
-    if (!isMutable)
-        newMap.state_ = SHAMapState::Immutable;
-
-    newMap.cowid_ = cowid_ + 1;
-    newMap.ledgerSeq_ = ledgerSeq_;
-    newMap.root_ = root_;
-    newMap.backed_ = backed_;
-
-    if ((state_ != SHAMapState::Immutable) ||
-        (newMap.state_ != SHAMapState::Immutable))
-    {
-        // If either map may change, they cannot share nodes
-        newMap.unshare();
-    }
-
-    return ret;
+    return std::make_shared<SHAMap>(*this, isMutable);
 }

 void
@@ -174,7 +174,6 @@ SHAMap::finishFetch(
 {
    assert(backed_);

-    std::shared_ptr<SHAMapTreeNode> node;
    try
    {
        if (!object)
@@ -187,27 +186,23 @@ SHAMap::finishFetch(
            return {};
        }

-        node =
+        auto node =
            SHAMapTreeNode::makeFromPrefix(makeSlice(object->getData()), hash);
        if (node)
            canonicalize(hash, node);
        return node;
    }
-
-    catch (SHAMapMissingNode const& e)
-    {
-        JLOG(journal_.warn()) << "Missing node: " << hash << " : " << e.what();
-    }
    catch (std::runtime_error const& e)
    {
-        JLOG(journal_.warn()) << __func__ << " : " << e.what();
+        JLOG(journal_.warn()) << "finishFetch exception: " << e.what();
    }
    catch (...)
    {
-        JLOG(journal_.warn()) << "Invalid DB node " << hash;
+        JLOG(journal_.warn())
+            << "finishFetch exception: unknonw exception: " << hash;
    }

-    return std::shared_ptr<SHAMapTreeNode>();
+    return {};
 }

 // See if a sync filter has a node