Skip to content

Commit

Permalink
Merge pull request #3895 from dmkozh/write_fee_fix
Browse files Browse the repository at this point in the history 
Stop counting the key size for read/write bytes.

Reviewed-by: sisuresh
  • Loading branch information
@latobarita
latobarita authored Aug 25, 2023
2 parents c5b96ca + 01d5c31 commit a4d6230
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 23 deletions.
5 changes: 2 additions & 3 deletions src/transactions/BumpFootprintExpirationOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ BumpFootprintExpirationOpFrame::doApply(Application& app,
continue;
}

auto keySize = static_cast<uint32>(xdr::xdr_size(lk));
auto entrySize = static_cast<uint32>(xdr::xdr_size(ltxe.current()));

metrics.mLedgerReadByte += keySize + entrySize;
metrics.mLedgerReadByte += entrySize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
innerResult().code(
Expand All @@ -99,7 +98,7 @@ BumpFootprintExpirationOpFrame::doApply(Application& app,
rustEntryRentChanges.emplace_back();
auto& rustChange = rustEntryRentChanges.back();
rustChange.is_persistent = !isTemporaryEntry(lk);
rustChange.old_size_bytes = static_cast<uint32>(keySize + entrySize);
rustChange.old_size_bytes = static_cast<uint32>(entrySize);
rustChange.new_size_bytes = rustChange.old_size_bytes;
rustChange.old_expiration_ledger = currExpiration;
rustChange.new_expiration_ledger = bumpLedger;
Expand Down
13 changes: 6 additions & 7 deletions src/transactions/InvokeHostFunctionOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,15 +192,15 @@ struct HostFunctionMetrics
mReadEntry++;
mReadKeyByte += keySize;
mMaxReadWriteKeyByte = std::max(mMaxReadWriteKeyByte, keySize);
mLedgerReadByte += keySize + entrySize;
mLedgerReadByte += entrySize;
if (isCodeEntry)
{
mReadCodeByte += keySize + entrySize;
mReadCodeByte += entrySize;
mMaxReadWriteCodeByte = std::max(mMaxReadWriteCodeByte, entrySize);
}
else
{
mReadDataByte += keySize + entrySize;
mReadDataByte += entrySize;
mMaxReadWriteDataByte = std::max(mMaxReadWriteDataByte, entrySize);
}
}
Expand All @@ -209,17 +209,16 @@ struct HostFunctionMetrics
noteWriteEntry(bool isCodeEntry, uint32 keySize, uint32 entrySize)
{
mWriteEntry++;
mWriteKeyByte += keySize;
mMaxReadWriteKeyByte = std::max(mMaxReadWriteKeyByte, keySize);
mLedgerWriteByte += keySize + entrySize;
mLedgerWriteByte += entrySize;
if (isCodeEntry)
{
mWriteCodeByte += keySize + entrySize;
mWriteCodeByte += entrySize;
mMaxReadWriteCodeByte = std::max(mMaxReadWriteCodeByte, entrySize);
}
else
{
mWriteDataByte += keySize + entrySize;
mWriteDataByte += entrySize;
mMaxReadWriteDataByte = std::max(mMaxReadWriteDataByte, entrySize);
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/transactions/RestoreFootprintOpFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
ledgerSeq + expirationSettings.minPersistentEntryExpiration - 1;
for (auto const& lk : footprint.readWrite)
{
auto keySize = static_cast<uint32>(xdr::xdr_size(lk));
uint32_t entrySize = UINT32_MAX;
{
auto const_ltxe = ltx.loadWithoutRecord(lk);
Expand All @@ -84,7 +83,7 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,

entrySize =
static_cast<uint32>(xdr::xdr_size(const_ltxe.current()));
metrics.mLedgerReadByte += keySize + entrySize;
metrics.mLedgerReadByte += entrySize;
if (resources.readBytes < metrics.mLedgerReadByte)
{
innerResult().code(RESTORE_FOOTPRINT_RESOURCE_LIMIT_EXCEEDED);
Expand All @@ -103,7 +102,7 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
auto ltxe = ltx.load(lk);

auto& restoredEntry = ltxe.current();
metrics.mLedgerWriteByte += keySize + entrySize;
metrics.mLedgerWriteByte += entrySize;

if (resources.writeBytes < metrics.mLedgerWriteByte ||
resources.readBytes < metrics.mLedgerReadByte)
Expand All @@ -119,7 +118,7 @@ RestoreFootprintOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx,
// for the rent fee purposes.
rustChange.old_size_bytes = 0;
rustChange.old_expiration_ledger = 0;
rustChange.new_size_bytes = keySize + entrySize;
rustChange.new_size_bytes = entrySize;
rustChange.new_expiration_ledger = restoredExpirationLedger;
setExpirationLedger(restoredEntry, restoredExpirationLedger);
}
Expand Down
18 changes: 9 additions & 9 deletions src/transactions/test/InvokeHostFunctionTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
{
// Restore Instance and WASM
restoreOp(contractKeys,
125 /* rent bump */ + 40000 /* two LE-writes */);
123 /* rent bump */ + 40000 /* two LE-writes */);

// Instance should now be useable
putWithFootprint(
Expand All @@ -1126,7 +1126,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
{
// Only restore contract instance
restoreOp({contractKeys[0]},
39 /* rent bump */ + 20000 /* one LE write */);
38 /* rent bump */ + 20000 /* one LE write */);

// invocation should fail
putWithFootprint(
Expand All @@ -1146,7 +1146,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
{
// Only restore WASM
restoreOp({contractKeys[1]},
87 /* rent bump */ + 20000 /* one LE write */);
86 /* rent bump */ + 20000 /* one LE write */);

// invocation should fail
putWithFootprint(
Expand All @@ -1166,16 +1166,16 @@ TEST_CASE("contract storage", "[tx][soroban]")
{
// Restore Instance and WASM
restoreOp(contractKeys,
125 /* rent bump */ + 40000 /* two LE writes */);
123 /* rent bump */ + 40000 /* two LE writes */);

auto instanceBumpAmount = 10'000;
auto wasmBumpAmount = 15'000;

// bump instance
bumpOp(instanceBumpAmount, {contractKeys[0]}, 20040);
bumpOp(instanceBumpAmount, {contractKeys[0]}, 20039);

// bump WASM
bumpOp(wasmBumpAmount, {contractKeys[1]}, 20171);
bumpOp(wasmBumpAmount, {contractKeys[1]}, 20170);

checkKeyExpirationLedger(contractKeys[0], ledgerSeq,
ledgerSeq + instanceBumpAmount);
Expand Down Expand Up @@ -1329,7 +1329,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
ContractDataDurability::PERSISTENT, DATA_ENTRY),
contractDataKey(contractID, makeSymbolSCVal("key3"),
ContractDataDurability::PERSISTENT, DATA_ENTRY)},
40075); // only 2 ledger writes because key3 won't be bumped
40074); // only 2 ledger writes because key3 won't be bumped

checkContractDataExpirationLedger(
"key", ContractDataDurability::PERSISTENT, ledgerSeq + 10'100);
Expand All @@ -1349,7 +1349,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
1;

// Bump instance and WASM so that they don't expire during the test
bumpOp(10'000, contractKeys, 40148);
bumpOp(10'000, contractKeys, 40147);

put("key", 0, ContractDataDurability::PERSISTENT);
checkContractDataExpirationLedger(
Expand Down Expand Up @@ -1392,7 +1392,7 @@ TEST_CASE("contract storage", "[tx][soroban]")
"key", ContractDataDurability::PERSISTENT, initExpirationLedger);

// Restore the entry
restoreOp({lk}, 20039);
restoreOp({lk}, 20038);

ledgerSeq = getLedgerSeq(*app);
checkContractDataExpirationState(
Expand Down

0 comments on commit a4d6230

Please sign in to comment.