Skip to content

Commit 829233a

Browse files
marta-lokhovamarta-lokhova
committed
Address feedback from code review
1 parent 8d76128 commit 829233a

File tree

8 files changed

+41
-59
lines changed

8 files changed

+41
-59
lines changed

‎src/invariant/ArchivedStateConsistency.cpp‎

Lines changed: 23 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,8 @@ std::string
9595
ArchivedStateConsistency::checkOnLedgerCommit(
9696
SearchableSnapshotConstPtr lclLiveState,
9797
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
98-
std::vector<LedgerEntry> const& evictedFromLive,
99-
std::vector<LedgerKey> const& deletedKeysFromLive,
98+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
99+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
100100
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
101101
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState)
102102
{
@@ -108,10 +108,6 @@ ArchivedStateConsistency::checkOnLedgerCommit(
108108
lclLiveState->getLedgerHeader().ledgerVersion,
109109
LiveBucket::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION))
110110
{
111-
CLOG_INFO(Invariant,
112-
"Skipping ArchivedStateConsistency invariant for "
113-
"protocol version {}",
114-
lclLiveState->getLedgerHeader().ledgerVersion);
115111
return std::string{};
116112
}
117113
auto ledgerSeq = lclLiveState->getLedgerSeq() + 1;
@@ -121,42 +117,28 @@ ArchivedStateConsistency::checkOnLedgerCommit(
121117
LedgerKeySet allKeys;
122118

123119
// Keys for evicted from live entries
124-
for (auto const& e : evictedFromLive)
120+
for (auto const& e : persitentEvictedFromLive)
125121
{
126122
auto key = LedgerEntryKey(e);
123+
releaseAssertOrThrow(isPersistentEntry(key));
127124
allKeys.insert(key);
128-
if (isPersistentEntry(key))
129-
{
130-
allKeys.insert(getTTLKey(e));
131-
}
132125
}
133126

134127
// Keys for deleted from live (temp and TTLs)
135-
for (auto const& k : deletedKeysFromLive)
128+
for (auto const& k : tempAndTTLEvictedFromLive)
136129
{
130+
releaseAssertOrThrow(!isPersistentEntry(k));
137131
allKeys.insert(k);
138-
if (isPersistentEntry(k))
139-
{
140-
allKeys.insert(getTTLKey(k));
141-
}
142132
}
143133

144134
// Keys for restored entries
145135
for (auto const& [key, entry] : restoredFromArchive)
146136
{
147137
allKeys.insert(key);
148-
if (isPersistentEntry(key))
149-
{
150-
allKeys.insert(getTTLKey(entry));
151-
}
152138
}
153139
for (auto const& [key, entry] : restoredFromLiveState)
154140
{
155141
allKeys.insert(key);
156-
if (isPersistentEntry(key))
157-
{
158-
allKeys.insert(getTTLKey(entry));
159-
}
160142
}
161143

162144
// Preload from both live and archived state
@@ -180,14 +162,14 @@ ArchivedStateConsistency::checkOnLedgerCommit(
180162
}
181163

182164
UnorderedSet<LedgerKey> deletedKeys;
183-
for (auto const& k : deletedKeysFromLive)
165+
for (auto const& k : tempAndTTLEvictedFromLive)
184166
{
185167
deletedKeys.insert(k);
186168
}
187169

188170
auto evictionRes = checkEvictionInvariants(
189171
preloadedLiveEntries, preloadedArchivedEntries, deletedKeys,
190-
evictedFromLive, ledgerSeq, ledgerVers);
172+
persitentEvictedFromLive, ledgerSeq, ledgerVers);
191173

192174
auto restoreRes = checkRestoreInvariants(
193175
preloadedLiveEntries, preloadedArchivedEntries, restoredFromArchive,
@@ -450,20 +432,22 @@ ArchivedStateConsistency::checkRestoreInvariants(
450432
// Skip this check prior to protocol 24, since there was a bug in 23
451433
// Don't check lastModifiedLedgerSeq, since it may have been updated by
452434
// the ltx
453-
else if (!(hotArchiveEntry->second.archivedEntry().data ==
454-
entry.data) ||
455-
!(hotArchiveEntry->second.archivedEntry().ext == entry.ext) &&
456-
protocolVersionStartsFrom(ledgerVer,
457-
ProtocolVersion::V_24))
435+
else if (protocolVersionStartsFrom(ledgerVer, ProtocolVersion::V_24))
458436
{
459-
return fmt::format(
460-
FMT_STRING(
461-
"ArchivedStateConsistency invariant failed: "
462-
"Restored entry from archive has incorrect value: Entry to "
463-
"Restore: {}, Hot Archive Entry: {}"),
464-
xdrToCerealString(entry, "entry_to_restore"),
465-
xdrToCerealString(hotArchiveEntry->second.archivedEntry(),
466-
"hot_archive_entry"));
437+
bool isValid =
438+
hotArchiveEntry->second.archivedEntry().data == entry.data &&
439+
hotArchiveEntry->second.archivedEntry().ext == entry.ext;
440+
if (!isValid)
441+
{
442+
return fmt::format(
443+
FMT_STRING("ArchivedStateConsistency invariant failed: "
444+
"Restored entry from archive has incorrect "
445+
"value: Entry to "
446+
"Restore: {}, Hot Archive Entry: {}"),
447+
xdrToCerealString(entry, "entry_to_restore"),
448+
xdrToCerealString(hotArchiveEntry->second.archivedEntry(),
449+
"hot_archive_entry"));
450+
}
467451
}
468452
}
469453

‎src/invariant/ArchivedStateConsistency.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ class ArchivedStateConsistency : public Invariant
3636
virtual std::string checkOnLedgerCommit(
3737
SearchableSnapshotConstPtr lclLiveState,
3838
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
39-
std::vector<LedgerEntry> const& evictedFromLive,
40-
std::vector<LedgerKey> const& deletedKeysFromLive,
39+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
40+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
4141
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
4242
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState)
4343
override;

‎src/invariant/Invariant.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,8 @@ class Invariant
7676
checkOnLedgerCommit(
7777
SearchableSnapshotConstPtr lclLiveState,
7878
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
79-
std::vector<LedgerEntry> const& evictedFromLive,
80-
std::vector<LedgerKey> const& deletedKeysFromLive,
79+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
80+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
8181
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
8282
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState)
8383
{

‎src/invariant/InvariantManager.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ class InvariantManager
5555
virtual void checkOnLedgerCommit(
5656
SearchableSnapshotConstPtr lclLiveState,
5757
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
58-
std::vector<LedgerEntry> const& evictedFromLive,
59-
std::vector<LedgerKey> deletedKeysFromLive,
58+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
59+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
6060
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
6161
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState) = 0;
6262

‎src/invariant/InvariantManagerImpl.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,16 +166,17 @@ void
166166
InvariantManagerImpl::checkOnLedgerCommit(
167167
SearchableSnapshotConstPtr lclLiveState,
168168
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
169-
std::vector<LedgerEntry> const& evictedFromLive,
170-
std::vector<LedgerKey> deletedKeysFromLive,
169+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
170+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
171171
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
172172
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState)
173173
{
174174
for (auto invariant : mEnabled)
175175
{
176176
auto result = invariant->checkOnLedgerCommit(
177-
lclLiveState, lclHotArchiveState, evictedFromLive,
178-
deletedKeysFromLive, restoredFromArchive, restoredFromLiveState);
177+
lclLiveState, lclHotArchiveState, persitentEvictedFromLive,
178+
tempAndTTLEvictedFromLive, restoredFromArchive,
179+
restoredFromLiveState);
179180
if (result.empty())
180181
{
181182
continue;
@@ -311,7 +312,7 @@ InvariantManagerImpl::start(Application& app)
311312
}
312313

313314
auto message = fmt::format(
314-
FMT_STRING(R"(Invariant "{}" does not hold on ledger commit: {})"),
315+
FMT_STRING(R"(Invariant "{}" does not hold on startup: {})"),
315316
invariant->getName(), result);
316317
onInvariantFailure(invariant, message,
317318
app.getLedgerManager().getLastClosedLedgerNum());

‎src/invariant/InvariantManagerImpl.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ class InvariantManagerImpl : public InvariantManager
5252
virtual void checkOnLedgerCommit(
5353
SearchableSnapshotConstPtr lclLiveState,
5454
SearchableHotArchiveSnapshotConstPtr lclHotArchiveState,
55-
std::vector<LedgerEntry> const& evictedFromLive,
56-
std::vector<LedgerKey> deletedKeysFromLive,
55+
std::vector<LedgerEntry> const& persitentEvictedFromLive,
56+
std::vector<LedgerKey> const& tempAndTTLEvictedFromLive,
5757
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromArchive,
5858
UnorderedMap<LedgerKey, LedgerEntry> const& restoredFromLiveState)
5959
override;

‎src/invariant/test/InvariantTests.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,12 +331,12 @@ TEST_CASE_VERSIONS("State archival eviction invariant", "[invariant][archival]")
331331
// Create test entries that will remain live
332332
auto [liveTempEntry, liveTempTTL] =
333333
createContractDataWithTTL(TEMPORARY, evictionLedger + 100);
334-
auto [livePeristentEntry, livePersistentTTL] =
334+
auto [livePersistentEntry, livePersistentTTL] =
335335
createContractDataWithTTL(PERSISTENT, evictionLedger + 100);
336336

337337
lm.setNextLedgerEntryBatchForBucketTesting(
338338
{tempEntry, tempTTL, persistentEntry, persistentTTL, liveTempEntry,
339-
liveTempTTL, livePeristentEntry, livePersistentTTL},
339+
liveTempTTL, livePersistentEntry, livePersistentTTL},
340340
{}, {});
341341
closeLedger(*app);
342342

@@ -484,9 +484,9 @@ TEST_CASE_VERSIONS("State archival eviction invariant", "[invariant][archival]")
484484
{
485485
// Add live data key and TTL to evicted state (they shouldn't be
486486
// evicted since TTL is not expired)
487-
evictedState.archivedEntries.push_back(livePeristentEntry);
487+
evictedState.archivedEntries.push_back(livePersistentEntry);
488488
evictedState.deletedKeys.push_back(
489-
getTTLKey(livePeristentEntry));
489+
getTTLKey(livePersistentEntry));
490490

491491
REQUIRE_THROWS_AS(
492492
app->getInvariantManager().checkOnLedgerCommit(

‎src/ledger/LedgerManagerImpl.cpp‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1581,9 +1581,6 @@ LedgerManagerImpl::applyLedger(LedgerCloseData const& ledgerData,
15811581
auto ledgerSeq = ltx.loadHeader().current().ledgerSeq;
15821582

15831583
auto& appConnector = mApp.getAppConnector();
1584-
bool isP24UpgradeLedger =
1585-
protocolVersionIsBefore(initialLedgerVers, ProtocolVersion::V_24) &&
1586-
protocolVersionStartsFrom(maybeNewVersion, ProtocolVersion::V_24);
15871584
auto appliedLedgerState = sealLedgerTxnAndStoreInBucketsAndDB(
15881585
appConnector.copySearchableLiveBucketListSnapshot(),
15891586
appConnector.copySearchableHotArchiveBucketListSnapshot(), ltx,

0 commit comments

Comments
 (0)