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

Fix meta #3844

Closed
wants to merge 4 commits into from
Closed

Fix meta #3844

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3098455
Emit update instead of create on restore
sisuresh Jul 20, 2023
3188a29
Allow overwritting expired temp entries
sisuresh Jul 20, 2023
69c37b4
Check expiration in getNewestVersion
sisuresh Jul 20, 2023
5c6b07a
Only load in restoreFootprint if entry is being restored.
sisuresh Jul 21, 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
10 changes: 1 addition & 9 deletions src/ledger/InMemoryLedgerTxn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,22 +233,14 @@ 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)
{
throw std::runtime_error("called erase on InMemoryLedgerTxn");
}

LedgerTxnEntry
InMemoryLedgerTxn::load(InternalLedgerKey const& key)
InMemoryLedgerTxn::load(InternalLedgerKey const& key, bool loadExpiredEntry)
{
throw std::runtime_error("called load on InMemoryLedgerTxn");
}
Expand Down
7 changes: 2 additions & 5 deletions src/ledger/InMemoryLedgerTxn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,9 @@ class InMemoryLedgerTxn : public LedgerTxn

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;
LedgerTxnEntry load(InternalLedgerKey const& key,
bool loadExpiredEntry) override;
ConstLedgerTxnEntry loadWithoutRecord(InternalLedgerKey const& key,
bool loadExpiredEntry) override;

Expand Down
152 changes: 58 additions & 94 deletions src/ledger/LedgerTxn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,18 +605,23 @@ LedgerTxn::create(InternalLedgerEntry const& entry)
return getImpl()->create(*this, entry);
}

#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
LedgerTxnEntry
LedgerTxn::restore(InternalLedgerEntry const& entry)
LedgerTxn::Impl::create(LedgerTxn& self, InternalLedgerEntry const& entry)
{
return getImpl()->restore(*this, entry);
}
#endif
throwIfSealed();
throwIfChild();

auto key = entry.toKey();

// For entries that can be restored, we need to check the key for both
// expired entries and live entries
auto checkExpired = key.type() == InternalLedgerEntryType::LEDGER_ENTRY &&
isRestorableEntry(key.ledgerKey());
if (getNewestVersion(key, checkExpired))
{
throw std::runtime_error("Key already exists");
}

LedgerTxnEntry
LedgerTxn::Impl::createRestoreCommon(LedgerTxn& self,
InternalLedgerEntry const& entry)
{
auto current = std::make_shared<InternalLedgerEntry>(entry);
auto impl = LedgerTxnEntry::makeSharedImpl(self, *current);

Expand All @@ -638,76 +643,6 @@ LedgerTxn::Impl::createRestoreCommon(LedgerTxn& self,
return ltxe;
}

LedgerTxnEntry
LedgerTxn::Impl::create(LedgerTxn& self, InternalLedgerEntry const& entry)
{
throwIfSealed();
throwIfChild();

auto key = entry.toKey();

// For entries that can be restored, we need to check the key for both
// expired entries and live entries
auto checkExpired = key.type() == InternalLedgerEntryType::LEDGER_ENTRY &&
isRestorableEntry(key.ledgerKey());
if (getNewestVersion(key, checkExpired))
{
throw std::runtime_error("Key already exists");
}

return createRestoreCommon(self, entry);
}

#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
LedgerTxnEntry
LedgerTxn::Impl::restore(LedgerTxn& self, InternalLedgerEntry const& entry)
{
throwIfSealed();
throwIfChild();

auto key = entry.toKey();

if (!isRestorableEntry(key.ledgerKey()))
{
throw std::runtime_error(
"Restored entry that is not a restorable type");
}

auto expiredVersion = getNewestVersion(key, /*loadExpiredEntry=*/true);
if (!expiredVersion)
{
throw std::runtime_error("Restored entry that does not exist");
}

if (getNewestVersion(key, /*loadExpiredEntry=*/false))
{
throw std::runtime_error("Restored live entry");
}

// Check that data fields of expired version and new version are identical.
bool integrityCheck;
auto const& expiredLE = expiredVersion->ledgerEntry();
if (key.ledgerKey().type() == CONTRACT_DATA)
{
integrityCheck = expiredLE.data.contractData().body ==
entry.ledgerEntry().data.contractData().body;
}
else
{
integrityCheck = expiredLE.data.contractCode().body ==
entry.ledgerEntry().data.contractCode().body;
}

if (!integrityCheck)
{
throw std::runtime_error(
"Restored version has different data than expired version");
}

return createRestoreCommon(self, entry);
}
#endif

void
LedgerTxn::createWithoutLoading(InternalLedgerEntry const& entry)
{
Expand Down Expand Up @@ -812,7 +747,7 @@ LedgerTxn::Impl::erase(InternalLedgerKey const& key)
throwIfSealed();
throwIfChild();

auto newest = getNewestVersionEntryMap(key);
auto newest = getNewestVersionEntryMap(key, false);
if (!newest.first)
{
throw std::runtime_error("Key does not exist");
Expand Down Expand Up @@ -1219,7 +1154,7 @@ LedgerTxn::Impl::getChanges()
else
{
auto previous =
mParent.getNewestVersion(key, /*loadExpiredEntry=*/false);
mParent.getNewestVersion(key, /*loadExpiredEntry=*/true);
// entry is not init, so previous must exist. If not, then
// we're modifying an entry that doesn't exist.
releaseAssert(previous);
Expand Down Expand Up @@ -1512,29 +1447,57 @@ LedgerTxn::getNewestVersion(InternalLedgerKey const& key,
return getImpl()->getNewestVersion(key, loadExpiredEntry);
}

static std::shared_ptr<InternalLedgerEntry const>
checkExpiration(std::shared_ptr<InternalLedgerEntry const> ile_ptr,
int32_t ledgerSeq, bool loadExpiredEntry)
{
#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
if (!ile_ptr || ile_ptr->type() != InternalLedgerEntryType::LEDGER_ENTRY)
{
return ile_ptr;
}

if (loadExpiredEntry || isLive(ile_ptr->ledgerEntry(), ledgerSeq))
{
return ile_ptr;
}
else
{
return nullptr;
}
#else
return ile_ptr;
#endif
}

std::shared_ptr<InternalLedgerEntry const>
LedgerTxn::Impl::getNewestVersion(InternalLedgerKey const& key,
bool loadExpiredEntry) const
{
auto iter = mEntry.find(key);
if (iter != mEntry.end())
{
return iter->second.get();
return checkExpiration(iter->second.get(), mHeader->ledgerSeq,
loadExpiredEntry);
}
return mParent.getNewestVersion(key, loadExpiredEntry);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add an expiration check above in LedgerTxn::Impl::getNewestVersion now that mEntry may contain expired entries since load may make an expired entry active. This also applies in getNewestVersionEntryMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed

std::pair<std::shared_ptr<InternalLedgerEntry const>,
LedgerTxn::Impl::EntryMap::iterator>
LedgerTxn::Impl::getNewestVersionEntryMap(InternalLedgerKey const& key)
LedgerTxn::Impl::getNewestVersionEntryMap(InternalLedgerKey const& key,
bool loadExpiredEntry)
{
auto iter = mEntry.find(key);
if (iter != mEntry.end())
{
return std::make_pair(iter->second.get(), iter);
return std::make_pair(checkExpiration(iter->second.get(),
mHeader->ledgerSeq,
loadExpiredEntry),
iter);
}
return std::make_pair(
mParent.getNewestVersion(key, /*loadExpiredEntry=*/false), iter);
return std::make_pair(mParent.getNewestVersion(key, loadExpiredEntry),
iter);
}

UnorderedMap<LedgerKey, LedgerEntry>
Expand Down Expand Up @@ -1645,13 +1608,14 @@ LedgerTxn::Impl::getPoolShareTrustLinesByAccountAndAsset(
}

LedgerTxnEntry
LedgerTxn::load(InternalLedgerKey const& key)
LedgerTxn::load(InternalLedgerKey const& key, bool loadExpiredEntry)
{
return getImpl()->load(*this, key);
return getImpl()->load(*this, key, loadExpiredEntry);
}

LedgerTxnEntry
LedgerTxn::Impl::load(LedgerTxn& self, InternalLedgerKey const& key)
LedgerTxn::Impl::load(LedgerTxn& self, InternalLedgerKey const& key,
bool loadExpiredEntry)
{
throwIfSealed();
throwIfChild();
Expand All @@ -1660,7 +1624,7 @@ LedgerTxn::Impl::load(LedgerTxn& self, InternalLedgerKey const& key)
throw std::runtime_error("Key is active");
}

auto newest = getNewestVersionEntryMap(key);
auto newest = getNewestVersionEntryMap(key, loadExpiredEntry);
if (!newest.first)
{
return {};
Expand Down Expand Up @@ -1717,7 +1681,7 @@ LedgerTxn::Impl::loadAllOffers(LedgerTxn& self)
{
auto const& key = kv.first;
auto const& sellerID = key.offer().sellerID;
offersByAccount[sellerID].emplace_back(load(self, key));
offersByAccount[sellerID].emplace_back(load(self, key, false));
}
return offersByAccount;
}
Expand Down Expand Up @@ -1746,7 +1710,7 @@ LedgerTxn::Impl::loadBestOffer(LedgerTxn& self, Asset const& buying,
throwIfChild();

auto le = getBestOffer(buying, selling);
auto res = le ? load(self, LedgerEntryKey(*le)) : LedgerTxnEntry();
auto res = le ? load(self, LedgerEntryKey(*le), false) : LedgerTxnEntry();

try
{
Expand Down Expand Up @@ -1863,7 +1827,7 @@ LedgerTxn::Impl::loadOffersByAccountAndAsset(LedgerTxn& self,
for (auto const& kv : offers)
{
auto const& key = kv.first;
res.emplace_back(load(self, key));
res.emplace_back(load(self, key, false));
}
return res;
}
Expand Down Expand Up @@ -1902,7 +1866,7 @@ LedgerTxn::Impl::loadPoolShareTrustLinesByAccountAndAsset(
for (auto const& kv : trustLines)
{
auto const& key = kv.first;
res.emplace_back(load(self, key));
res.emplace_back(load(self, key, false));
}
return res;
}
Expand Down
23 changes: 6 additions & 17 deletions src/ledger/LedgerTxn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
// access this entry in this LedgerTxn." See below for the
// concurrency-control issues this is designed to trap.
//
// - Entries are made-active by calling load(), create(), or restore(), each
// - Entries are made-active by calling load(), or create(), each
// of which returns a LedgerTxnEntry which is a handle that can be used to
// get at the underlying LedgerEntry. References to the underlying
// LedgerEntries should generally not be retained anywhere, because the
Expand Down Expand Up @@ -541,7 +541,7 @@ class AbstractLedgerTxn : public AbstractLedgerTxnParent
virtual void commit() noexcept = 0;
virtual void rollback() noexcept = 0;

// loadHeader, create, restore, erase, load, and loadWithoutRecord provide
// loadHeader, create, erase, load, and loadWithoutRecord provide
// the main interface to interact with data stored in the AbstractLedgerTxn.
// These functions only allow one instance of a particular data to be active
// at a time.
Expand All @@ -552,11 +552,6 @@ class AbstractLedgerTxn : public AbstractLedgerTxnParent
// Creates a new LedgerTxnEntry from entry. Throws if the key
// associated with this entry is already associated with an entry in
// this AbstractLedgerTxn or any parent.
// - restore
// Creates a new LedgerTxnEntry from expired entry. Throws if the key
// associated with this entry is already associated with an entry in
// this AbstractLedgerTxn or any parent or if the expired entry being
// restored does not exist
// - erase
// Erases the existing entry associated with key. Throws if the key is
// not already associated with an entry in this AbstractLedgerTxn or
Expand All @@ -578,12 +573,9 @@ class AbstractLedgerTxn : public AbstractLedgerTxnParent
virtual LedgerTxnHeader loadHeader() = 0;
virtual LedgerTxnEntry create(InternalLedgerEntry const& entry) = 0;

#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
virtual LedgerTxnEntry restore(InternalLedgerEntry const& entry) = 0;
#endif

virtual void erase(InternalLedgerKey const& key) = 0;
virtual LedgerTxnEntry load(InternalLedgerKey const& key) = 0;
virtual LedgerTxnEntry load(InternalLedgerKey const& key,
bool loadExpiredEntry = false) = 0;
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 made this an optional parameter in the interest of time. We're going to remove this anyways so it should be fine.

virtual ConstLedgerTxnEntry loadWithoutRecord(InternalLedgerKey const& key,
bool loadExpiredEntry) = 0;

Expand Down Expand Up @@ -716,10 +708,6 @@ class LedgerTxn : public AbstractLedgerTxn

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;

UnorderedMap<LedgerKey, LedgerEntry> getAllOffers() override;
Expand Down Expand Up @@ -760,7 +748,8 @@ class LedgerTxn : public AbstractLedgerTxn
getNewestVersion(InternalLedgerKey const& key,
bool loadExpiredEntry) const override;

LedgerTxnEntry load(InternalLedgerKey const& key) override;
LedgerTxnEntry load(InternalLedgerKey const& key,
bool loadExpiredEntry = false) override;

void createWithoutLoading(InternalLedgerEntry const& entry) override;
void updateWithoutLoading(InternalLedgerEntry const& entry) override;
Expand Down
20 changes: 4 additions & 16 deletions src/ledger/LedgerTxnImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,12 +438,8 @@ class LedgerTxn::Impl

// lookup in mEntry or in parents
std::pair<std::shared_ptr<InternalLedgerEntry const>, EntryMap::iterator>
getNewestVersionEntryMap(InternalLedgerKey const& key);

// Common logic for create and restore code paths. Input correctness
// checking should be done before calling this function
LedgerTxnEntry createRestoreCommon(LedgerTxn& self,
InternalLedgerEntry const& entry);
getNewestVersionEntryMap(InternalLedgerKey const& key,
bool loadExpiredEntry);

public:
// Constructor has the strong exception safety guarantee
Expand All @@ -464,15 +460,6 @@ class LedgerTxn::Impl
// - the entry cache may be, but is not guaranteed to be, cleared.
LedgerTxnEntry create(LedgerTxn& self, InternalLedgerEntry const& entry);

// restore has the basic exception safety guarantee. If it throws an
// exception, then
// - the prepared statement cache may be, but is not guaranteed to be,
// modified
// - the entry cache may be, but is not guaranteed to be, cleared.
#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
LedgerTxnEntry restore(LedgerTxn& self, InternalLedgerEntry const& entry);
#endif

// deactivate has the strong exception safety guarantee
void deactivate(InternalLedgerKey const& key);

Expand Down Expand Up @@ -573,7 +560,8 @@ class LedgerTxn::Impl
// - the prepared statement cache may be, but is not guaranteed to be,
// modified
// - the entry cache may be, but is not guaranteed to be, cleared.
LedgerTxnEntry load(LedgerTxn& self, InternalLedgerKey const& key);
LedgerTxnEntry load(LedgerTxn& self, InternalLedgerKey const& key,
bool loadExpiredEntry);

// createWithoutLoading has the strong exception safety guarantee.
// If it throws an exception, then the current LedgerTxn::Impl is unchanged.
Expand Down
Loading