Skip to content

Commit

Permalink
Merge pull request #4009 from sisuresh/reads
Browse files Browse the repository at this point in the history 
Count read bytes for expired temp entries

Reviewed-by: dmkozh
  • Loading branch information
@latobarita
latobarita authored Nov 2, 2023
2 parents 031a6db + ba91936 commit af8b65a
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 117 deletions.
67 changes: 30 additions & 37 deletions src/transactions/ExtendFootprintTTLOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,58 +66,50 @@ ExtendFootprintTTLOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
uint32_t newLiveUntilLedgerSeq = ledgerSeq + mExtendFootprintTTLOp.extendTo;
for (auto const& lk : footprint.readOnly)
{
// Load the ContractCode/ContractData entry for fee calculation.
auto entryLtxe = ltx.loadWithoutRecord(lk);
if (!entryLtxe)
{
// Skip the missing entries. Since this happens at apply
// time and we refund the unspent fees, it is more beneficial
// to extend as many entries as possible.
continue;
}

auto ttlKey = getTTLKey(lk);
uint32_t entrySize = UINT32_MAX;
uint32_t ttlSize = UINT32_MAX;
uint32_t currLiveUntilLedgerSeq = UINT32_MAX;

{
// Initially load without record since we may not need to modify
// entry
auto ttlConstLtxe = ltx.loadWithoutRecord(ttlKey);
releaseAssertOrThrow(ttlConstLtxe);
if (!isLive(ttlConstLtxe.current(), ledgerSeq))
if (!ttlConstLtxe || !isLive(ttlConstLtxe.current(), ledgerSeq))
{
// Also skip archived entries, as those must be restored
// Skip archived entries, as those must be restored.
//
// Also skip the missing entries. Since this happens at apply
// time and we refund the unspent fees, it is more beneficial
// to extend as many entries as possible.
continue;
}

entrySize = static_cast<uint32>(xdr::xdr_size(entryLtxe.current()));
ttlSize =
static_cast<uint32>(xdr::xdr_size(ttlConstLtxe.current()));

metrics.mLedgerReadByte += entrySize + ttlSize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
mParentTx.pushSimpleDiagnosticError(
SCE_BUDGET, SCEC_EXCEEDED_LIMIT,
"operation byte-read resources exceeds amount specified",
{makeU64SCVal(metrics.mLedgerReadByte),
makeU64SCVal(resources.readBytes)});

innerResult().code(
EXTEND_FOOTPRINT_TTL_RESOURCE_LIMIT_EXCEEDED);
return false;
}

currLiveUntilLedgerSeq =
auto currLiveUntilLedgerSeq =
ttlConstLtxe.current().data.ttl().liveUntilLedgerSeq;
if (currLiveUntilLedgerSeq >= newLiveUntilLedgerSeq)
{
continue;
}
}

// Load the ContractCode/ContractData entry for fee calculation.
auto entryLtxe = ltx.loadWithoutRecord(lk);

// We checked for TTLEntry existence above
releaseAssertOrThrow(entryLtxe);

uint32_t entrySize =
static_cast<uint32>(xdr::xdr_size(entryLtxe.current()));
metrics.mLedgerReadByte += entrySize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
mParentTx.pushSimpleDiagnosticError(
SCE_BUDGET, SCEC_EXCEEDED_LIMIT,
"operation byte-read resources exceeds amount specified",
{makeU64SCVal(metrics.mLedgerReadByte),
makeU64SCVal(resources.readBytes)});

innerResult().code(EXTEND_FOOTPRINT_TTL_RESOURCE_LIMIT_EXCEEDED);
return false;
}

// We already checked that the TTLEntry exists in the logic above
auto ttlLtxe = ltx.load(ttlKey);

Expand All @@ -126,7 +118,8 @@ ExtendFootprintTTLOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
rustChange.is_persistent = !isTemporaryEntry(lk);
rustChange.old_size_bytes = static_cast<uint32>(entrySize);
rustChange.new_size_bytes = rustChange.old_size_bytes;
rustChange.old_live_until_ledger = currLiveUntilLedgerSeq;
rustChange.old_live_until_ledger =
ttlLtxe.current().data.ttl().liveUntilLedgerSeq;
rustChange.new_live_until_ledger = newLiveUntilLedgerSeq;
ttlLtxe.current().data.ttl().liveUntilLedgerSeq = newLiveUntilLedgerSeq;
}
Expand Down
78 changes: 39 additions & 39 deletions src/transactions/InvokeHostFunctionOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,

// Get the entries for the footprint
rust::Vec<CxxBuf> ledgerEntryCxxBufs;
rust::Vec<CxxBuf> TTLEntryCxxBufs;
rust::Vec<CxxBuf> ttlEntryCxxBufs;

auto const& resources = mParentTx.sorobanResources();
auto const& footprint = resources.footprint;
Expand All @@ -382,69 +382,69 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,

ledgerEntryCxxBufs.reserve(footprintLength);

auto addReads = [&ledgerEntryCxxBufs, &TTLEntryCxxBufs, &ltx, &metrics,
auto addReads = [&ledgerEntryCxxBufs, &ttlEntryCxxBufs, &ltx, &metrics,
&resources, this](auto const& keys) -> bool {
for (auto const& lk : keys)
{
uint32 keySize = static_cast<uint32>(xdr::xdr_size(lk));
uint32 entrySize = 0u;
// Load without record for readOnly to avoid writing them later
auto ltxe = ltx.loadWithoutRecord(lk);
if (ltxe)
{
bool shouldAddEntry = true;
auto const& le = ltxe.current();
std::optional<TTLEntry> TTLEntry = std::nullopt;
std::optional<TTLEntry> ttlEntry = std::nullopt;
bool sorobanEntryLive = false;

// For soroban entries, check if the entry is archived
if (isSorobanEntry(le.data))
// For soroban entries, check if the entry is expired before loading
if (isSorobanEntry(lk))
{
auto ttlKey = getTTLKey(lk);
auto ttlLtxe = ltx.loadWithoutRecord(ttlKey);
if (ttlLtxe)
{
auto ttlKey = getTTLKey(lk);
auto ttlLtxe = ltx.loadWithoutRecord(ttlKey);
releaseAssertOrThrow(ttlLtxe);
if (!isLive(ttlLtxe.current(), ltx.getHeader().ledgerSeq))
{
if (isTemporaryEntry(lk))
{
// For temporary entries, treat the non-live entry
// as if the key did not exist
shouldAddEntry = false;
}
else
// For temporary entries, treat the expired entry as
// if the key did not exist
if (!isTemporaryEntry(lk))
{
// Cannot access an archived entry
this->innerResult().code(
INVOKE_HOST_FUNCTION_ENTRY_ARCHIVED);
return false;
}
}

TTLEntry = ttlLtxe.current().data.ttl();
else
{
sorobanEntryLive = true;
ttlEntry = ttlLtxe.current().data.ttl();
}
}
// If ttlLtxe doesn't exist, this is a new Soroban entry
}

if (shouldAddEntry)
if (!isSorobanEntry(lk) || sorobanEntryLive)
{
auto ltxe = ltx.loadWithoutRecord(lk);
if (ltxe)
{
auto leBuf = toCxxBuf(le);
auto leBuf = toCxxBuf(ltxe.current());
entrySize = static_cast<uint32>(leBuf.data->size());

// For entry types that don't have an TTLEntry (i.e.
// For entry types that don't have an ttlEntry (i.e.
// Accounts), the rust host expects an "empty" CxxBuf such
// that the buffer has a non-null pointer that points to an
// empty byte vector
auto ttlBuf =
TTLEntry
? toCxxBuf(*TTLEntry)
ttlEntry
? toCxxBuf(*ttlEntry)
: CxxBuf{std::make_unique<std::vector<uint8_t>>()};

entrySize = static_cast<uint32>(leBuf.data->size());
if (TTLEntry)
{
entrySize += static_cast<uint32_t>(ttlBuf.data->size());
}

ledgerEntryCxxBufs.emplace_back(std::move(leBuf));
TTLEntryCxxBufs.emplace_back(std::move(ttlBuf));
ttlEntryCxxBufs.emplace_back(std::move(ttlBuf));
}
else if (isSorobanEntry(lk))
{
releaseAssertOrThrow(!ttlEntry);
}
}

metrics.noteReadEntry(isCodeKey(lk), keySize, entrySize);

if (resources.readBytes < metrics.mLedgerReadByte)
Expand Down Expand Up @@ -498,7 +498,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
toCxxBuf(mInvokeHostFunction.hostFunction), toCxxBuf(resources),
toCxxBuf(getSourceID()), authEntryCxxBufs,
getLedgerInfo(ltx, cfg, sorobanConfig), ledgerEntryCxxBufs,
TTLEntryCxxBufs, basePrngSeedBuf,
ttlEntryCxxBufs, basePrngSeedBuf,
sorobanConfig.rustBridgeRentFeeConfiguration());
metrics.mCpuInsn = static_cast<uint32>(out.cpu_insns);
metrics.mMemByte = static_cast<uint32>(out.mem_bytes);
Expand Down Expand Up @@ -559,7 +559,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
uint32 keySize = static_cast<uint32>(xdr::xdr_size(lk));
uint32 entrySize = static_cast<uint32>(buf.data.size());

// TTLEntry write fees come out of refundableFee, already
// ttlEntry write fees come out of refundableFee, already
// accounted for by the host
if (lk.type() != TTL)
{
Expand Down Expand Up @@ -590,7 +590,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
}

// Check that each newly created ContractCode or ContractData entry also
// creates an TTLEntry
// creates an ttlEntry
for (auto const& key : createdKeys)
{
if (isSorobanEntry(key))
Expand Down Expand Up @@ -618,7 +618,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
releaseAssertOrThrow(isSorobanEntry(lk));
ltx.erase(lk);

// Also delete associated TTLEntry
// Also delete associated ttlEntry
auto ttlLK = getTTLKey(lk);
auto ttlLtxe = ltx.load(ttlLK);
releaseAssertOrThrow(ttlLtxe);
Expand Down
56 changes: 18 additions & 38 deletions src/transactions/RestoreFootprintOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,54 +73,26 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
ledgerSeq + archivalSettings.minPersistentTTL - 1;
for (auto const& lk : footprint.readWrite)
{
uint32_t entrySize = UINT32_MAX;
uint32_t ttlSize = UINT32_MAX;

// We must load the ContractCode/ContractData entry for fee purposes, as
// restore is considered a write
auto constEntryLtxe = ltx.loadWithoutRecord(lk);
if (!constEntryLtxe)
{
continue;
}

auto ttlKey = getTTLKey(lk);
{
auto constTTLLtxe = ltx.loadWithoutRecord(ttlKey);
releaseAssertOrThrow(constTTLLtxe);

entrySize =
static_cast<uint32>(xdr::xdr_size(constEntryLtxe.current()));
ttlSize =
static_cast<uint32>(xdr::xdr_size(constTTLLtxe.current()));

metrics.mLedgerReadByte += entrySize + ttlSize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
mParentTx.pushSimpleDiagnosticError(
SCE_BUDGET, SCEC_EXCEEDED_LIMIT,
"operation byte-read resources exceeds amount specified",
{makeU64SCVal(metrics.mLedgerReadByte),
makeU64SCVal(resources.readBytes)});
innerResult().code(RESTORE_FOOTPRINT_RESOURCE_LIMIT_EXCEEDED);
return false;
}

if (isLive(constTTLLtxe.current(), ledgerSeq))
// Skip entry if the TTLEntry is missing or if it's already live.
if (!constTTLLtxe || isLive(constTTLLtxe.current(), ledgerSeq))
{
// Skip entries that are already live.
continue;
}
}

// Entry exists if we get this this point due to the loadWithoutRecord
// logic above.
auto ttlLtxe = ltx.load(ttlKey);
// We must load the ContractCode/ContractData entry for fee purposes, as
// restore is considered a write
auto constEntryLtxe = ltx.loadWithoutRecord(lk);

// To maintain consistency with InvokeHostFunction, TTLEntry
// writes come out of refundable fee, so only add entrySize
metrics.mLedgerWriteByte += entrySize;
// We checked for TTLEntry existence above
releaseAssertOrThrow(constEntryLtxe);

uint32_t entrySize =
static_cast<uint32>(xdr::xdr_size(constEntryLtxe.current()));
metrics.mLedgerReadByte += entrySize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
mParentTx.pushSimpleDiagnosticError(
Expand All @@ -132,6 +104,10 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
return false;
}

// To maintain consistency with InvokeHostFunction, TTLEntry
// writes come out of refundable fee, so only add entrySize
metrics.mLedgerWriteByte += entrySize;

if (resources.writeBytes < metrics.mLedgerWriteByte)
{
mParentTx.pushSimpleDiagnosticError(
Expand All @@ -152,6 +128,10 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
rustChange.old_live_until_ledger = 0;
rustChange.new_size_bytes = entrySize;
rustChange.new_live_until_ledger = restoredLiveUntilLedger;

// Entry exists if we get this this point due to the constTTLLtxe
// loadWithoutRecord logic above.
auto ttlLtxe = ltx.load(ttlKey);
ttlLtxe.current().data.ttl().liveUntilLedgerSeq =
restoredLiveUntilLedger;
}
Expand Down
6 changes: 3 additions & 3 deletions src/transactions/test/InvokeHostFunctionTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,18 +371,18 @@ getRentFeeForExtend(Application& app, LedgerKey const& key,
auto ttlLtxe = ltx.loadWithoutRecord(ttlKey);
releaseAssert(ttlLtxe);

TTLEntry const& TTLEntry = ttlLtxe.current().data.ttl();
TTLEntry const& ttlEntry = ttlLtxe.current().data.ttl();

uint32_t numLedgersToExtend = 0;
if (isLive(ttlLtxe.current(), ledgerSeq))
{
auto ledgerToExtendTo = ledgerSeq + newLifetime;
if (TTLEntry.liveUntilLedgerSeq >= ledgerToExtendTo)
if (ttlEntry.liveUntilLedgerSeq >= ledgerToExtendTo)
{
return 0;
}

numLedgersToExtend = ledgerToExtendTo - TTLEntry.liveUntilLedgerSeq;
numLedgersToExtend = ledgerToExtendTo - ttlEntry.liveUntilLedgerSeq;
}
else
{
Expand Down

0 comments on commit af8b65a

Please sign in to comment.