Cleanup how restoredKeys are tracked in LedgerTxn

src/invariant/ArchivedStateConsistency.cpp

@@ -428,7 +428,7 @@ ArchivedStateConsistency::checkRestoreInvariants(
                            "in live state: {}"),
                 xdrToCerealString(key, "key"));
         }
-        else if (liveEntry->second != entry)
+        else if (key.type() != TTL && liveEntry->second != entry)
         {
             return fmt::format(
                 FMT_STRING("ArchivedStateConsistency invariant failed: "
@@ -438,14 +438,24 @@ ArchivedStateConsistency::checkRestoreInvariants(
                 xdrToCerealString(entry, "entry_to_restore"));
         }
 
-        if (key.type() == TTL && isLive(entry, ledgerSeq))
+        if (key.type() == TTL)
         {
-            return fmt::format(
-                FMT_STRING("ArchivedStateConsistency invariant failed: "
-                           "Restored entry from live BucketList is not "
-                           "expired: Entry: {}, TTL Entry: {}"),
-                xdrToCerealString(entry, "entry"),
-                xdrToCerealString(entry, "ttl_entry"));
+            if (!isLive(entry, ledgerSeq))
+            {
+                return fmt::format(
+                    FMT_STRING("ArchivedStateConsistency invariant failed: "
+                               "Restored entries updated state is still "
+                               "expired: TTL Entry: {}"),
+                    xdrToCerealString(entry, "ttl_entry"));
+            }
+            if (isLive(liveEntry->second, ledgerSeq))
+            {
+                return fmt::format(
+                    FMT_STRING("ArchivedStateConsistency invariant failed: "
+                               "Restored entry from live BucketList is not "
+                               "expired: TTL Entry: {}"),
+                    xdrToCerealString(liveEntry->second, "ttl_entry"));
+            }
         }
     }
 

src/ledger/LedgerTxn.cpp

@@ -251,11 +251,10 @@ RestoredEntries::addLiveBucketlistRestore(LedgerKey const& key,
 }
 
 void
-RestoredEntries::addRestoresFrom(RestoredEntries const& other,
-                                 bool allowDuplicates)
+RestoredEntries::addRestoresFrom(RestoredEntries const& other)
 {
     ZoneScoped;
-    // This method is called from three different call sites. In 2 of them it is
+    // This method is called from three different call sites. In all three it is
     // correct to assert that each restore is new/disjoint from any existing
     // restore:
     //
@@ -272,19 +271,19 @@ RestoredEntries::addRestoresFrom(RestoredEntries const& other,
     //     entry -- it'd be a concurrency bug if not! -- so there should not be
     //     any other restores of the same entry from other threads.
     //
-    // In the third place we're committing from an ltx to its parent, and the
-    // ltx was actually starting with a copy of the restored-maps from the
-    // parent, so there are going to be duplicates. We allow duplicates in that
-    // case.
-    for (auto kvp : other.hotArchive)
+    //   - In the third call site we're committing from a child ltx to its
+    //     parent. Since child LedgerTxns only track their own restores (they
+    //     do not start with a copy of the parent's restored entries), there
+    //     should be no duplicates.
+    for (auto const& kvp : other.hotArchive)
     {
         auto [_, inserted] = hotArchive.emplace(kvp.first, kvp.second);
-        releaseAssert(inserted || allowDuplicates);
+        releaseAssert(inserted);
     }
-    for (auto kvp : other.liveBucketList)
+    for (auto const& kvp : other.liveBucketList)
     {
         auto [_, inserted] = liveBucketList.emplace(kvp.first, kvp.second);
-        releaseAssert(inserted || allowDuplicates);
+        releaseAssert(inserted);
     }
 }
 
@@ -435,15 +434,6 @@ LedgerTxn::Impl::Impl(LedgerTxn& self, AbstractLedgerTxnParent& parent,
     , mConsistency(LedgerTxnConsistency::EXACT)
     , mActiveThreadId(std::this_thread::get_id())
 {
-    for (auto const& [key, entry] : mParent.getRestoredHotArchiveKeys())
-    {
-        mRestoredEntries.hotArchive.emplace(key, entry);
-    }
-    for (auto const& [key, entry] : mParent.getRestoredLiveBucketListKeys())
-    {
-        mRestoredEntries.liveBucketList.emplace(key, entry);
-    }
-
     mParent.addChild(self, mode);
 }
 
@@ -703,10 +693,7 @@ LedgerTxn::Impl::commitChild(EntryIterator iter,
         printErrorAndAbort("unknown fatal error during commit to LedgerTxn");
     }
 
-    // The child will have started with a copy of the parents mRestoredEntries,
-    // so we can see duplicates here, but duplicate restores would've been
-    // caught during restoration in the restoreFrom* functions.
-    mRestoredEntries.addRestoresFrom(restoredEntries, /*allowDuplicates=*/true);
+    mRestoredEntries.addRestoresFrom(restoredEntries);
 
     // std::unique_ptr<...>::swap does not throw
     mHeader.swap(childHeader);
@@ -915,6 +902,41 @@ LedgerTxn::Impl::markRestoredFromHotArchive(LedgerEntry const& ledgerEntry,
     addKey(ttlEntry);
 }
 
+void
+LedgerTxn::markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                          LedgerEntry const& ttlEntry)
+{
+    getImpl()->markRestoredFromLiveBucketList(ledgerEntry, ttlEntry);
+}
+
+void
+LedgerTxn::Impl::markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                                LedgerEntry const& ttlEntry)
+{
+    abortIfWrongThread("markRestoredFromLiveBucketList");
+    throwIfSealed();
+    throwIfChild();
+
+    if (!isPersistentEntry(ledgerEntry.data))
+    {
+        throw std::runtime_error(
+            "Key type not supported for live BucketList restore");
+    }
+
+    // Mark the keys as restored
+    auto addKey = [this](LedgerEntry const& entry) {
+        auto [_, inserted] = mRestoredEntries.liveBucketList.emplace(
+            LedgerEntryKey(entry), entry);
+        if (!inserted)
+        {
+            throw std::runtime_error(
+                "Key already restored from Live BucketList");
+        }
+    };
+    addKey(ledgerEntry);
+    addKey(ttlEntry);
+}
+
 LedgerTxnEntry
 LedgerTxn::restoreFromLiveBucketList(LedgerEntry const& entry, uint32_t ttl)
 {

src/ledger/LedgerTxn.h

@@ -359,8 +359,7 @@ struct RestoredEntries
                                   LedgerEntry const& entry,
                                   LedgerKey const& ttlKey,
                                   LedgerEntry const& ttlEntry);
-    void addRestoresFrom(RestoredEntries const& other,
-                         bool allowDuplicates = false);
+    void addRestoresFrom(RestoredEntries const& other);
 };
 
 class AbstractLedgerTxn;
@@ -617,6 +616,13 @@ class AbstractLedgerTxn : public AbstractLedgerTxnParent
     //     restored. This just adds the information to the map tracking entries
     //     restored from the hot archive. The actual restoration of the entry is
     //     handled separately.
+    // - markRestoredFromLiveBucketList:
+    //     Indicates that an entry in the live BucketList is being restored.
+    //     Used by the parallel apply path to signal to LedgerTxn that the
+    //     entry and TTL should be treated as if they have been restored. This
+    //     just adds the information to the map tracking entries restored from
+    //     the live BucketList. The actual restoration of the entry is handled
+    //     separately.
     // All of these functions throw if the AbstractLedgerTxn is sealed or if
     // the AbstractLedgerTxn has a child.
     virtual LedgerTxnHeader loadHeader() = 0;
@@ -626,6 +632,9 @@ class AbstractLedgerTxn : public AbstractLedgerTxnParent
                                                      uint32_t ttl) = 0;
     virtual void markRestoredFromHotArchive(LedgerEntry const& ledgerEntry,
                                             LedgerEntry const& ttlEntry) = 0;
+    virtual void
+    markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                   LedgerEntry const& ttlEntry) = 0;
     virtual LedgerTxnEntry load(InternalLedgerKey const& key) = 0;
     virtual ConstLedgerTxnEntry
     loadWithoutRecord(InternalLedgerKey const& key) = 0;
@@ -774,6 +783,8 @@ class LedgerTxn : public AbstractLedgerTxn
                                              uint32_t ttl) override;
     void markRestoredFromHotArchive(LedgerEntry const& ledgerEntry,
                                     LedgerEntry const& ttlEntry) override;
+    void markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                        LedgerEntry const& ttlEntry) override;
 
     UnorderedMap<LedgerKey, LedgerEntry> getAllOffers() override;
 

src/ledger/LedgerTxnImpl.h

@@ -360,9 +360,18 @@ class LedgerTxn::Impl
 
     // markRestoredFromHotArchive has the basic exception safety guarantee. If
     // it throws an exception, then
+    // - the restored entries map may contain only a partial record (e.g. the
+    //   data entry without its corresponding TTL entry).
     void markRestoredFromHotArchive(LedgerEntry const& ledgerEntry,
                                     LedgerEntry const& ttlEntry);
 
+    // markRestoredFromLiveBucketList has the basic exception safety guarantee.
+    // If it throws an exception, then
+    // - the restored entries map may contain only a partial record (e.g. the
+    //   data entry without its corresponding TTL entry).
+    void markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                        LedgerEntry const& ttlEntry);
+
     // restoreFromLiveBucketList has the basic exception safety guarantee. If it
     // throws an exception, then
     LedgerTxnEntry restoreFromLiveBucketList(LedgerTxn& self,

src/ledger/test/InMemoryLedgerTxn.cpp

@@ -282,6 +282,14 @@ InMemoryLedgerTxn::restoreFromLiveBucketList(LedgerEntry const& entry,
         "called restoreFromLiveBucketList on InMemoryLedgerTxn");
 }
 
+void
+InMemoryLedgerTxn::markRestoredFromLiveBucketList(
+    LedgerEntry const& ledgerEntry, LedgerEntry const& ttlEntry)
+{
+    throw std::runtime_error(
+        "called markRestoredFromLiveBucketList on InMemoryLedgerTxn");
+}
+
 LedgerTxnEntry
 InMemoryLedgerTxn::load(InternalLedgerKey const& key)
 {

src/ledger/test/InMemoryLedgerTxn.h

@@ -114,6 +114,8 @@ class InMemoryLedgerTxn : public LedgerTxn
     void erase(InternalLedgerKey const& key) override;
     LedgerTxnEntry restoreFromLiveBucketList(LedgerEntry const& entry,
                                              uint32_t ttl) override;
+    void markRestoredFromLiveBucketList(LedgerEntry const& ledgerEntry,
+                                        LedgerEntry const& ttlEntry) override;
     LedgerTxnEntry load(InternalLedgerKey const& key) override;
     ConstLedgerTxnEntry
     loadWithoutRecord(InternalLedgerKey const& key) override;

src/transactions/ParallelApplyUtils.cpp

@@ -426,7 +426,10 @@ GlobalParallelApplyLedgerState::commitChangesToLedgerTxn(
 
     // While the final state of a restored key that will be written to the
     // Live BucketList is already handled in mGlobalEntryMap, we need to
-    // let the ltx know what keys need to be removed from the Hot Archive.
+    // let the ltx know what keys were restored so that:
+    // 1. Hot Archive restores can be removed from the Hot Archive BucketList
+    // 2. The ArchivedStateConsistency invariant can validate both hot archive
+    //    and live BucketList restores
     for (auto const& kvp : mGlobalRestoredEntries.hotArchive)
     {
         // We will search for the ttl key in the hot archive when the entry
@@ -439,6 +442,17 @@ GlobalParallelApplyLedgerState::commitChangesToLedgerTxn(
             ltxInner.markRestoredFromHotArchive(kvp.second, it->second);
         }
     }
+    for (auto const& kvp : mGlobalRestoredEntries.liveBucketList)
+    {
+        if (kvp.first.type() != TTL)
+        {
+            auto it = mGlobalRestoredEntries.liveBucketList.find(
+                getTTLKey(kvp.first));
+            releaseAssertOrThrow(it !=
+                                 mGlobalRestoredEntries.liveBucketList.end());
+            ltxInner.markRestoredFromLiveBucketList(kvp.second, it->second);
+        }
+    }
     ltxInner.commit();
 }
 

src/transactions/TransactionMeta.cpp

@@ -362,32 +362,11 @@ OperationMetaBuilder::setLedgerChanges(AbstractLedgerTxn& opLtx,
             opLtx.getHeader().ledgerVersion, ProtocolVersion::V_23));
     }
 
-    // getRestoredHotArchiveKeys and getRestoredLiveBucketListKeys return all
-    // entries that have been restored this ledger, not just by this op.
-    // However, processOpLedgerEntryChanges expects just the map of restores for
-    // this op. This function only gets called for <p23, so we only have to
-    // worry about the restore op. We look at the TTLs that have been modified
-    // by this op (i.e. restored TTLs) and use that to create an op-specific
-    // subset of the restored key maps.
-    UnorderedMap<LedgerKey, LedgerEntry> opRestoredLiveBucketListKeys{};
-    auto allRestoredLiveBucketListKeys = opLtx.getRestoredLiveBucketListKeys();
-    auto opModifiedKeys = opLtx.getAllKeysWithoutSealing();
-    if (mOp.getOperation().body.type() == OperationType::RESTORE_FOOTPRINT)
-    {
-        for (auto const& [key, entry] : allRestoredLiveBucketListKeys)
-        {
-            if (isSorobanEntry(key))
-            {
-                auto ttlKey = getTTLKey(key);
-                if (opModifiedKeys.find(ttlKey) != opModifiedKeys.end())
-                {
-                    opRestoredLiveBucketListKeys[key] = entry;
-                    opRestoredLiveBucketListKeys[ttlKey] =
-                        allRestoredLiveBucketListKeys.at(ttlKey);
-                }
-            }
-        }
-    }
+    // We should only have restored live BucketList keys for the restore
+    // operation pre-v23
+    auto opRestoredLiveBucketListKeys = opLtx.getRestoredLiveBucketListKeys();
+    releaseAssertOrThrow(opRestoredLiveBucketListKeys.empty() ||
+                         opType == OperationType::RESTORE_FOOTPRINT);
 
     // Note: Hot Archive restore map is always empty since this is never called
     // in p23.