Merge pull request #3857 from SirTyson/move-expiration

Move expiration enforcement to OpFrame level

Reviewed-by: sisuresh

src/bucket/Bucket.cpp

@@ -919,44 +919,19 @@ mergeCasesWithEqualKeys(MergeCounters& mc, BucketInputIterator& oi,
 
     if (newEntry.type() == INITENTRY)
     {
-        // For all entries except TEMPORARY entries, the only legal new-is-INIT
-        // case is merging a delete+create to an update. For TEMPORARY entries,
-        // an INIT entry may merge with another INIT entry as long as the older
-        // INIT entry is expired. Because merging occurs on a background thread
-        // and different validators may start a merge at different times, it is
-        // not possible to accurately know the current ledgerSeq or to know if a
-        // given TEMPORARY entry has expired. Due to this, we don't check this
-        // invariant for TEMPORARY entries
-
-        // TODO: Add invariant check for TEMPORARY entries based on ledgerSeq
-        // when the given bucket started to merge
+        // The only legal new-is-INIT case is merging a delete+create to an
+        // update.
         if (oldEntry.type() != DEADENTRY)
         {
-#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
-            if (auto type = oldEntry.liveEntry().data.type();
-                type == CONTRACT_DATA || type == CONTRACT_CODE)
-            {
-                // Treat merge as if old entry did not exist
-                ++mc.mNewEntriesDefaultAccepted;
-                Bucket::checkProtocolLegality(newEntry, protocolVersion);
-                countNewEntryType(mc, newEntry);
-                maybePut(out, newEntry, shadowIterators,
-                         keepShadowedLifecycleEntries, mc);
-            }
-            else
-#endif
-                throw std::runtime_error(
-                    "Malformed bucket: old non-DEAD + new INIT.");
-        }
-        else
-        {
-            BucketEntry newLive;
-            newLive.type(LIVEENTRY);
-            newLive.liveEntry() = newEntry.liveEntry();
-            ++mc.mNewInitEntriesMergedWithOldDead;
-            maybePut(out, newLive, shadowIterators,
-                     keepShadowedLifecycleEntries, mc);
+            throw std::runtime_error(
+                "Malformed bucket: old non-DEAD + new INIT.");
         }
+        BucketEntry newLive;
+        newLive.type(LIVEENTRY);
+        newLive.liveEntry() = newEntry.liveEntry();
+        ++mc.mNewInitEntriesMergedWithOldDead;
+        maybePut(out, newLive, shadowIterators, keepShadowedLifecycleEntries,
+                 mc);
     }
     else if (oldEntry.type() == INITENTRY)
     {

src/bucket/BucketApplicator.cpp

@@ -125,10 +125,7 @@ BucketApplicator::advance(BucketApplicator::Counters& counters)
                     // Prior to protocol 11, INITENTRY didn't exist, so we need
                     // to check ltx to see if this is an update or a create
                     auto key = InternalLedgerEntry(e.liveEntry()).toKey();
-
-                    // Bucket Apply should process every entry in bucket
-                    // including expired ones to get the DB in the correct state
-                    if (ltx->getNewestVersion(key, /*loadExpiredEntry=*/true))
+                    if (ltx->getNewestVersion(key))
                     {
                         ltx->updateWithoutLoading(e.liveEntry());
                     }
@@ -158,8 +155,7 @@ BucketApplicator::advance(BucketApplicator::Counters& counters)
                 {
                     // Prior to protocol 11, DEAD entries could exist
                     // without LIVE entries in between
-                    if (ltx->getNewestVersion(e.deadEntry(),
-                                              /*loadExpiredEntry=*/true))
+                    if (ltx->getNewestVersion(e.deadEntry()))
                     {
                         ltx->eraseWithoutLoading(e.deadEntry());
                     }

src/bucket/test/BucketListTests.cpp

@@ -674,7 +674,7 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
             LedgerKey key(CONFIG_SETTING);
             key.configSetting().configSettingID =
                 ConfigSettingID::CONFIG_SETTING_BUCKETLIST_SIZE_WINDOW;
-            auto txle = ltx.loadWithoutRecord(key, /*loadExpiredEntry=*/false);
+            auto txle = ltx.loadWithoutRecord(key);
             releaseAssert(txle);
             auto const& leVector =
                 txle.current().data.configSetting().bucketListSizeWindow();
@@ -721,59 +721,6 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
         }
     });
 }
-
-TEST_CASE_VERSIONS("new temp entry merges with expired entry with same key",
-                   "[bucket][bucketlist]")
-{
-    VirtualClock clock;
-    Config cfg(getTestConfig());
-    Application::pointer app = createTestApplication(clock, cfg);
-    BucketList& bl = app->getBucketManager().getBucketList();
-    uint32_t ledgerSeq = 1;
-
-    for_versions_from(20, *app, [&] {
-        auto startingLedger = ledgerSeq;
-        auto tempEntry =
-            LedgerTestUtils::generateValidLedgerEntryOfType(CONTRACT_DATA);
-        setExpirationLedger(tempEntry, startingLedger + 4);
-        tempEntry.data.contractData().durability = TEMPORARY;
-
-        bl.addBatch(*app, ledgerSeq, getAppLedgerVersion(app), {tempEntry}, {},
-                    {});
-        ++ledgerSeq;
-
-        // Run BucketList until entry has expired
-        for (; ledgerSeq <= startingLedger + 4; ++ledgerSeq)
-        {
-            bl.addBatch(*app, ledgerSeq, getAppLedgerVersion(app), {}, {}, {});
-        }
-
-        setExpirationLedger(tempEntry, startingLedger + 500);
-        bl.addBatch(*app, ledgerSeq, getAppLedgerVersion(app), {tempEntry}, {},
-                    {});
-        ++ledgerSeq;
-
-        // Run BucketList until entry has expired
-        for (; ledgerSeq <= startingLedger + 1024; ++ledgerSeq)
-        {
-            bl.addBatch(*app, ledgerSeq, getAppLedgerVersion(app), {}, {}, {});
-        }
-
-        bool foundValidEntry = false;
-        auto b = bl.getLevel(5).getCurr();
-        for (BucketInputIterator iter(b); iter; ++iter)
-        {
-            auto entry = *iter;
-            if (entry.type() == INITENTRY && entry.liveEntry() == tempEntry)
-            {
-                foundValidEntry = true;
-                break;
-            }
-        }
-
-        REQUIRE(foundValidEntry);
-    });
-}
 #endif
 
 static std::string

src/herder/Upgrades.cpp

@@ -125,7 +125,7 @@ readMaxSorobanTxSetSize(AbstractLedgerTxn& ltx)
     LedgerKey key(LedgerEntryType::CONFIG_SETTING);
     key.configSetting().configSettingID =
         ConfigSettingID::CONFIG_SETTING_CONTRACT_EXECUTION_LANES;
-    return ltx.loadWithoutRecord(key, /*loadExpiredEntry=*/false)
+    return ltx.loadWithoutRecord(key)
         .current()
         .data.configSetting()
         .contractExecutionLanes()
@@ -852,7 +852,7 @@ getAvailableLimitExcludingLiabilities(AccountID const& accountID,
         LedgerKey key(TRUSTLINE);
         key.trustLine().accountID = accountID;
         key.trustLine().asset = assetToTrustLineAsset(asset);
-        auto trust = ltx.loadWithoutRecord(key, /*loadExpiredEntry=*/false);
+        auto trust = ltx.loadWithoutRecord(key);
         if (trust && isAuthorizedToMaintainLiabilities(trust))
         {
             auto const& tl = trust.current().data.trustLine();
@@ -1300,16 +1300,15 @@ ConfigUpgradeSetFrameConstPtr
 ConfigUpgradeSetFrame::makeFromKey(AbstractLedgerTxn& ltx,
                                    ConfigUpgradeSetKey const& key)
 {
-    auto ltxe = ltx.loadWithoutRecord(ConfigUpgradeSetFrame::getLedgerKey(key),
-                                      /*loadExpiredEntry=*/false);
-    if (!ltxe)
+    auto ltxe = ltx.loadWithoutRecord(ConfigUpgradeSetFrame::getLedgerKey(key));
+    if (!ltxe || !isLive(ltxe.current(), ltx.getHeader().ledgerSeq))
     {
         return nullptr;
     }
     auto const& contractData = ltxe.current().data.contractData();
     if (contractData.body.bodyType() != DATA_ENTRY ||
         contractData.body.data().val.type() != SCV_BYTES ||
-        contractData.durability != PERSISTENT)
+        contractData.durability != TEMPORARY)
     {
         return nullptr;
     }
@@ -1407,7 +1406,7 @@ ConfigUpgradeSetFrame::getLedgerKey(ConfigUpgradeSetKey const& upgradeKey)
     lk.contractData().contract.type(SC_ADDRESS_TYPE_CONTRACT);
     lk.contractData().contract.contractId() = upgradeKey.contractID;
     lk.contractData().key = v;
-    lk.contractData().durability = PERSISTENT;
+    lk.contractData().durability = TEMPORARY;
     return lk;
 }
 
@@ -1424,9 +1423,9 @@ ConfigUpgradeSetFrame::upgradeNeeded(AbstractLedgerTxn& ltx,
     {
         LedgerKey key(LedgerEntryType::CONFIG_SETTING);
         key.configSetting().configSettingID = updatedEntry.configSettingID();
-        bool isSame = ltx.loadWithoutRecord(key, /*loadExpiredEntry=*/false)
-                          .current()
-                          .data.configSetting() == updatedEntry;
+        bool isSame =
+            ltx.loadWithoutRecord(key).current().data.configSetting() ==
+            updatedEntry;
         if (!isSame)
         {
             return true;

src/herder/test/UpgradesTests.cpp

@@ -876,7 +876,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
             LedgerKey key(CONFIG_SETTING);
             key.configSetting().configSettingID =
                 ConfigSettingID::CONFIG_SETTING_CONTRACT_LEDGER_COST_V0;
-            auto le = ltx2.loadWithoutRecord(key, false).current();
+            auto le = ltx2.loadWithoutRecord(key).current();
             auto configSetting = le.data.configSetting();
             configSetting.contractLedgerCost().txMaxWriteBytes = upgradeVal;
 

src/invariant/BucketListIsConsistentWithDatabase.cpp

@@ -28,10 +28,7 @@ namespace stellar
 static std::string
 checkAgainstDatabase(AbstractLedgerTxn& ltx, LedgerEntry const& entry)
 {
-    // Database should contain same expried entries as BucketList even if they
-    // are not accesible
-    auto fromDb =
-        ltx.loadWithoutRecord(LedgerEntryKey(entry), /*loadExpiredEntry=*/true);
+    auto fromDb = ltx.loadWithoutRecord(LedgerEntryKey(entry));
     if (!fromDb)
     {
         std::string s{
@@ -56,9 +53,7 @@ checkAgainstDatabase(AbstractLedgerTxn& ltx, LedgerEntry const& entry)
 static std::string
 checkAgainstDatabase(AbstractLedgerTxn& ltx, LedgerKey const& key)
 {
-    // Database should contain same expried entries as BucketList even if they
-    // are not accesible
-    auto fromDb = ltx.loadWithoutRecord(key, /*loadExpiredEntry=*/true);
+    auto fromDb = ltx.loadWithoutRecord(key);
     if (!fromDb)
     {
         return {};

src/invariant/test/BucketListIsConsistentWithDatabaseTests.cpp

@@ -295,10 +295,8 @@ struct SelectBucketListGenerator : public BucketListGenerator
                 auto iter = filteredKeys.begin();
                 std::advance(iter, dist(gRandomEngine));
 
-                // For BucketList generation, expirationLedger shouldn't matter
                 mSelected = std::make_shared<LedgerEntry>(
-                    ltx.loadWithoutRecord(*iter, /*loadExpiredEntry=*/true)
-                        .current());
+                    ltx.loadWithoutRecord(*iter).current());
             }
         }
         return BucketListGenerator::generateLiveEntries(ltx);

src/ledger/InMemoryLedgerTxn.cpp

@@ -233,14 +233,6 @@ InMemoryLedgerTxn::create(InternalLedgerEntry const& entry)
     throw std::runtime_error("called create on InMemoryLedgerTxn");
 }
 
-#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
-LedgerTxnEntry
-InMemoryLedgerTxn::restore(InternalLedgerEntry const& entry)
-{
-    throw std::runtime_error("called restore on InMemoryLedgerTxn");
-}
-#endif
-
 void
 InMemoryLedgerTxn::erase(InternalLedgerKey const& key)
 {
@@ -254,8 +246,7 @@ InMemoryLedgerTxn::load(InternalLedgerKey const& key)
 }
 
 ConstLedgerTxnEntry
-InMemoryLedgerTxn::loadWithoutRecord(InternalLedgerKey const& key,
-                                     bool loadExpiredEntry)
+InMemoryLedgerTxn::loadWithoutRecord(InternalLedgerKey const& key)
 {
     throw std::runtime_error("called loadWithoutRecord on InMemoryLedgerTxn");
 }
@@ -280,7 +271,7 @@ InMemoryLedgerTxn::getOffersByAccountAndAsset(AccountID const& account,
             continue;
         }
 
-        auto newest = getNewestVersion(key, /*loadExpiredEntry=*/false);
+        auto newest = getNewestVersion(key);
         if (!newest)
         {
             throw std::runtime_error("Invalid ledger state");
@@ -317,10 +308,8 @@ InMemoryLedgerTxn::getPoolShareTrustLinesByAccountAndAsset(
             continue;
         }
 
-        auto pool = getNewestVersion(
-            liquidityPoolKey(
-                key.ledgerKey().trustLine().asset.liquidityPoolID()),
-            /*loadExpiredEntry=*/false);
+        auto pool = getNewestVersion(liquidityPoolKey(
+            key.ledgerKey().trustLine().asset.liquidityPoolID()));
         if (!pool)
         {
             throw std::runtime_error("Invalid ledger state");
@@ -330,7 +319,7 @@ InMemoryLedgerTxn::getPoolShareTrustLinesByAccountAndAsset(
         auto const& cp = lp.body.constantProduct();
         if (cp.params.assetA == asset || cp.params.assetB == asset)
         {
-            auto newest = getNewestVersion(key, /*loadExpiredEntry=*/false);
+            auto newest = getNewestVersion(key);
             if (!newest)
             {
                 throw std::runtime_error("Invalid ledger state");

src/ledger/InMemoryLedgerTxn.h

@@ -88,15 +88,10 @@ class InMemoryLedgerTxn : public LedgerTxn
     void eraseWithoutLoading(InternalLedgerKey const& key) override;
 
     LedgerTxnEntry create(InternalLedgerEntry const& entry) override;
-
-#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
-    LedgerTxnEntry restore(InternalLedgerEntry const& entry) override;
-#endif
-
     void erase(InternalLedgerKey const& key) override;
     LedgerTxnEntry load(InternalLedgerKey const& key) override;
-    ConstLedgerTxnEntry loadWithoutRecord(InternalLedgerKey const& key,
-                                          bool loadExpiredEntry) override;
+    ConstLedgerTxnEntry
+    loadWithoutRecord(InternalLedgerKey const& key) override;
 
     UnorderedMap<LedgerKey, LedgerEntry>
     getOffersByAccountAndAsset(AccountID const& account,

src/ledger/InMemoryLedgerTxnRoot.cpp

@@ -90,8 +90,7 @@ InMemoryLedgerTxnRoot::getInflationWinners(size_t maxWinners,
 }
 
 std::shared_ptr<InternalLedgerEntry const>
-InMemoryLedgerTxnRoot::getNewestVersion(InternalLedgerKey const& key,
-                                        bool loadExpiredEntry) const
+InMemoryLedgerTxnRoot::getNewestVersion(InternalLedgerKey const& key) const
 {
     return nullptr;
 }

src/ledger/InMemoryLedgerTxnRoot.h

@@ -62,8 +62,7 @@ class InMemoryLedgerTxnRoot : public AbstractLedgerTxnParent
     getInflationWinners(size_t maxWinners, int64_t minBalance) override;
 
     std::shared_ptr<InternalLedgerEntry const>
-    getNewestVersion(InternalLedgerKey const& key,
-                     bool loadExpiredEntry) const override;
+    getNewestVersion(InternalLedgerKey const& key) const override;
 
     uint64_t countObjects(LedgerEntryType let) const override;
     uint64_t countObjects(LedgerEntryType let,