Cleanup how restoredKeys are tracked in LedgerTxn (#5134)

sisuresh · web-flow · commit c69069ed14be · 2026-02-25T23:44:56.000Z
# Description Cleanup how restoredKeys are tracked in LedgerTxn  # Checklist - [ ] Reviewed the [contributing](https://github.com/stellar/stellar-core/blob/master/CONTRIBUTING.md#submitting-changes) document - [ ] Rebased on top of master (no merge commits) - [ ] Ran `clang-format` v8.0.0 (via `make format` or the Visual Studio extension) - [ ] Compiles - [ ] Ran all tests - [ ] If change impacts performance, include supporting evidence per the [performance document](https://github.com/stellar/stellar-core/blob/master/performance-eval/performance-eval.md)
diff --git a/src/invariant/ArchivedStateConsistency.cpp b/src/invariant/ArchivedStateConsistency.cpp
@@ -428,7 +428,13 @@ ArchivedStateConsistency::checkRestoreInvariants(
                            "in live state: {}"),
                 xdrToCerealString(key, "key"));
         }
-        else if (liveEntry->second != entry)
+        // For non-TTL entries, `entry` (the restored value) should be
+        // identical to `liveEntry->second` (the value on the live BucketList)
+        // since data/code entries are not modified during a restore. TTL
+        // entries are excluded from this check because restoration updates the
+        // TTL's liveUntilLedgerSeq, so the restored value will differ from the
+        // on-disk value.
+        else if (key.type() != TTL && liveEntry->second != entry)
         {
             return fmt::format(
                 FMT_STRING("ArchivedStateConsistency invariant failed: "
@@ -438,14 +444,28 @@ 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"));
+            // `entry` is the TTL after restoration (with updated
+            // liveUntilLedgerSeq). `liveEntry->second` is the original
+            // on-disk TTL before restoration. We check that the restored
+            // TTL is now live and the original was expired.
+            if (!isLive(entry, ledgerSeq))
+            {
+                return fmt::format(
+                    FMT_STRING("ArchivedStateConsistency invariant failed: "
+                               "Restored entry's updated TTL 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"));
+            }
         }
     }
 
diff --git a/src/ledger/LedgerTxn.cpp b/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)
 {
diff --git a/src/ledger/LedgerTxn.h b/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;
 
diff --git a/src/ledger/LedgerTxnImpl.h b/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,
diff --git a/src/ledger/test/InMemoryLedgerTxn.cpp b/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)
 {
diff --git a/src/ledger/test/InMemoryLedgerTxn.h b/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;
diff --git a/src/ledger/test/LedgerTxnTests.cpp b/src/ledger/test/LedgerTxnTests.cpp
@@ -261,18 +261,17 @@ TEST_CASE("LedgerTxn commit into LedgerTxn", "[ledgertxn]")
 
         SECTION("commited to parent")
         {
+            auto getTTLEntry = [](LedgerEntry const& entry,
+                                  uint32_t liveUntilLedgerSeq) -> LedgerEntry {
+                LedgerEntry ttl;
+                ttl.data.type(TTL);
+                ttl.data.ttl().liveUntilLedgerSeq = liveUntilLedgerSeq;
+                ttl.data.ttl().keyHash = getTTLKey(entry).ttl().keyHash;
+                return ttl;
+            };
+
             SECTION("hot archive")
             {
-                auto getTTLEntry =
-                    [](LedgerEntry const& entry,
-                       uint32_t liveUntilLedgerSeq) -> LedgerEntry {
-                    LedgerEntry ttl;
-                    ttl.data.type(TTL);
-                    ttl.data.ttl().liveUntilLedgerSeq = liveUntilLedgerSeq;
-                    ttl.data.ttl().keyHash = getTTLKey(entry).ttl().keyHash;
-                    return ttl;
-                };
-
                 ltx1.markRestoredFromHotArchive(
                     randomEntries[0], getTTLEntry(randomEntries[0], 42));
 
@@ -313,9 +312,51 @@ TEST_CASE("LedgerTxn commit into LedgerTxn", "[ledgertxn]")
                 }
             }
 
+            SECTION("mark live BL")
+            {
+                ltx1.markRestoredFromLiveBucketList(
+                    randomEntries[0], getTTLEntry(randomEntries[0], 42));
+
+                SECTION("rollback")
+                {
+                    {
+                        LedgerTxn ltx2(ltx1);
+                        ltx2.markRestoredFromLiveBucketList(
+                            randomEntries[1],
+                            getTTLEntry(randomEntries[1], 42));
+                    }
+
+                    REQUIRE(ltx1.getRestoredHotArchiveKeys().empty());
+                    auto keys = ltx1.getRestoredLiveBucketListKeys();
+
+                    // Data key + TTL
+                    REQUIRE(keys.size() == 2);
+                    checkKey(keys, randomKeys[0]);
+                }
+
+                SECTION("commit")
+                {
+                    {
+                        LedgerTxn ltx2(ltx1);
+                        ltx2.markRestoredFromLiveBucketList(
+                            randomEntries[1],
+                            getTTLEntry(randomEntries[1], 42));
+                        ltx2.commit();
+                    }
+
+                    REQUIRE(ltx1.getRestoredHotArchiveKeys().empty());
+                    auto keys = ltx1.getRestoredLiveBucketListKeys();
+
+                    // (data key + TTL) * 2
+                    REQUIRE(keys.size() == 4);
+                    checkKey(keys, randomKeys[0]);
+                    checkKey(keys, randomKeys[1]);
+                }
+            }
+
             SECTION("live BL")
             {
-                auto getTTLEntry = [](LedgerKey const& key) {
+                auto getTTLEntryFromKey = [](LedgerKey const& key) {
                     LedgerEntry ttl;
                     ttl.data.type(TTL);
                     ttl.data.ttl().liveUntilLedgerSeq = 42;
@@ -325,15 +366,15 @@ TEST_CASE("LedgerTxn commit into LedgerTxn", "[ledgertxn]")
 
                 // Populate live BL with key, then restore it
                 ltx1.create(randomEntries[0]);
-                ltx1.create(getTTLEntry(randomKeys[0]));
+                ltx1.create(getTTLEntryFromKey(randomKeys[0]));
                 ltx1.restoreFromLiveBucketList(randomEntries[0], 42);
 
                 SECTION("rollback")
                 {
                     {
                         LedgerTxn ltx2(ltx1);
                         ltx2.create(randomEntries[1]);
-                        ltx2.create(getTTLEntry(randomKeys[1]));
+                        ltx2.create(getTTLEntryFromKey(randomKeys[1]));
                         ltx2.restoreFromLiveBucketList(randomEntries[1], 42);
                     }
 
@@ -350,7 +391,7 @@ TEST_CASE("LedgerTxn commit into LedgerTxn", "[ledgertxn]")
                     {
                         LedgerTxn ltx2(ltx1);
                         ltx2.create(randomEntries[1]);
-                        ltx2.create(getTTLEntry(randomKeys[1]));
+                        ltx2.create(getTTLEntryFromKey(randomKeys[1]));
                         ltx2.restoreFromLiveBucketList(randomEntries[1], 42);
                         ltx2.commit();
                     }
diff --git a/src/transactions/ParallelApplyUtils.cpp b/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,20 @@ GlobalParallelApplyLedgerState::commitChangesToLedgerTxn(
             ltxInner.markRestoredFromHotArchive(kvp.second, it->second);
         }
     }
+    // Live BucketList restores are only tracked in LedgerTxn for the
+    // ArchivedStateConsistency invariant, but we unconditionally track it for
+    // now.
+    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();
 }
 
diff --git a/src/transactions/TransactionMeta.cpp b/src/transactions/TransactionMeta.cpp

Original file line number	Diff line number	Diff line change
`@@ -282,6 +282,14 @@ InMemoryLedgerTxn::restoreFromLiveBucketList(LedgerEntry const& entry,`
`282`	`282`	`"called restoreFromLiveBucketList on InMemoryLedgerTxn");`
`283`	`283`	`}`
`284`	`284`
	`285`	`+void`
	`286`	`+InMemoryLedgerTxn::markRestoredFromLiveBucketList(`
	`287`	`+ LedgerEntry const& ledgerEntry, LedgerEntry const& ttlEntry)`
	`288`	`+{`
	`289`	`+ throw std::runtime_error(`
	`290`	`+ "called markRestoredFromLiveBucketList on InMemoryLedgerTxn");`
	`291`	`+}`
	`292`	`+`
`285`	`293`	`LedgerTxnEntry`
`286`	`294`	`InMemoryLedgerTxn::load(InternalLedgerKey const& key)`
`287`	`295`	`{`