Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move expiration enforcement to OpFrame level #3857

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Move expiration enforcement to OpFrame level #3857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64aa506
Revert "Added LedgerTxn::restore"
SirTyson Jul 24, 2023
fa3d736
Revert "Add loadExpiredEntry flag and enforce expiration in mEntryCache"
SirTyson Jul 24, 2023
fde17a8
Restore emits update meta
SirTyson Jul 24, 2023
6a5e29f
Moved expiration enforcement to InvokeHostFunctionOpFrame
SirTyson Jul 25, 2023
60a8c64
Added expiration tests
SirTyson Jul 26, 2023
fadfcbe
Remove Sorban bucket invariant special case
SirTyson Jul 27, 2023
af0f81e
bumpOp skips expired entries
SirTyson Jul 27, 2023
0ad2053
Updated expiration settings for ConfigUpgrades
SirTyson Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 10 additions & 35 deletions src/bucket/Bucket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
8 changes: 2 additions & 6 deletions src/bucket/BucketApplicator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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());
}
Expand Down
55 changes: 1 addition & 54 deletions src/bucket/test/BucketListTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
19 changes: 9 additions & 10 deletions src/herder/Upgrades.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting... we don't check if the entry is live here, so we can technically upgrade using an expired entry. Should we keep this as is for more flexibility? Or should we make sure the entry is live? Checking that it's live sounds more consistent. cc @dmkozh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a great understanding of the upgrade system, but allowing anything to use an expired entry seems very sketchy (except for restoreOp of course). I think we should add a isLive check here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think we should need exceptions to the rules.

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;
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/herder/test/UpgradesTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
9 changes: 2 additions & 7 deletions src/invariant/BucketListIsConsistentWithDatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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 {};
Expand Down
4 changes: 1 addition & 3 deletions src/invariant/test/BucketListIsConsistentWithDatabaseTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
21 changes: 5 additions & 16 deletions src/ledger/InMemoryLedgerTxn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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");
}
Expand All @@ -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");
Expand Down Expand Up @@ -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");
Expand All @@ -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");
Expand Down
9 changes: 2 additions & 7 deletions src/ledger/InMemoryLedgerTxn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions src/ledger/InMemoryLedgerTxnRoot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions src/ledger/InMemoryLedgerTxnRoot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading