From 0a20c7ac1dd6715bc5355afcb4bd7af9572b2272 Mon Sep 17 00:00:00 2001 From: Siddharth Suresh Date: Fri, 22 Dec 2023 14:57:26 -0800 Subject: [PATCH 1/4] Trigger target size in the sorobanutils upgrade test and increase fees to handle it --- src/main/SettingsUpgradeUtils.cpp | 12 +++---- .../test/InvokeHostFunctionTests.cpp | 32 +++++++++++++++++-- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/main/SettingsUpgradeUtils.cpp b/src/main/SettingsUpgradeUtils.cpp index 0071680640..528545e72f 100644 --- a/src/main/SettingsUpgradeUtils.cpp +++ b/src/main/SettingsUpgradeUtils.cpp @@ -15,7 +15,7 @@ getUploadTx(PublicKey const& publicKey, SequenceNumber seqNum) auto& tx = txEnv.v1().tx; tx.sourceAccount = toMuxedAccount(publicKey); - tx.fee = 10'000'000; + tx.fee = 50'000'000; tx.seqNum = seqNum; Preconditions cond; @@ -49,7 +49,7 @@ getUploadTx(PublicKey const& publicKey, SequenceNumber seqNum) tx.ext.v(1); tx.ext.sorobanData().resources = uploadResources; - tx.ext.sorobanData().resourceFee = 4'000'000; + tx.ext.sorobanData().resourceFee = 35'000'000; return {txEnv, contractCodeLedgerKey}; } @@ -63,7 +63,7 @@ getCreateTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, auto& tx = txEnv.v1().tx; tx.sourceAccount = toMuxedAccount(publicKey); - tx.fee = 2'000'000; + tx.fee = 5'000'000; tx.seqNum = seqNum; Preconditions cond; @@ -133,7 +133,7 @@ getCreateTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, tx.ext.v(1); tx.ext.sorobanData().resources = uploadResources; - tx.ext.sorobanData().resourceFee = 1'000'000; + tx.ext.sorobanData().resourceFee = 3'000'000; return {txEnv, contractSourceRefLedgerKey, contractID}; } @@ -149,7 +149,7 @@ getInvokeTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, auto& tx = txEnv.v1().tx; tx.sourceAccount = toMuxedAccount(publicKey); - tx.fee = 1'000'000; + tx.fee = 100'000'000; tx.seqNum = seqNum; Preconditions cond; @@ -200,7 +200,7 @@ getInvokeTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, tx.ext.v(1); tx.ext.sorobanData().resources = invokeResources; - tx.ext.sorobanData().resourceFee = 500'000; + tx.ext.sorobanData().resourceFee = 65'000'000; ConfigUpgradeSetKey key; key.contentHash = upgradeHash; diff --git a/src/transactions/test/InvokeHostFunctionTests.cpp b/src/transactions/test/InvokeHostFunctionTests.cpp index 4fe298cdd6..a5f93290db 100644 --- a/src/transactions/test/InvokeHostFunctionTests.cpp +++ b/src/transactions/test/InvokeHostFunctionTests.cpp @@ -888,7 +888,6 @@ TEST_CASE("Soroban non-refundable resource fees are stable", "[tx][soroban]") int64_t expectedNonRefundableFee) { auto validTx = makeTx(resources, minInclusionFee, expectedNonRefundableFee); - auto txSize = xdr::xdr_size(validTx->getEnvelope()); auto& app = test.getApp(); // Sanity check the tx fee computation logic. auto actualFeePair = @@ -1615,7 +1614,6 @@ TEST_CASE("contract storage", "[tx][soroban]") auto appCfg = getTestConfig(); appCfg.ENABLE_SOROBAN_DIAGNOSTIC_EVENTS = true; SorobanTest test(appCfg, true, modifyCfg); - auto ledgerSeq = test.getLedgerSeq(); auto isSuccess = [](auto resultCode) { return resultCode == INVOKE_HOST_FUNCTION_SUCCESS; @@ -2487,6 +2485,36 @@ TEST_CASE("settings upgrade command line utils", "[tx][soroban][upgrades]") auto a1 = root.create("A", startingBalance); + // Update the snapshot period and close a ledger to update + // mAverageBucketListSize + app->getLedgerManager() + .getMutableSorobanNetworkConfig() + .setBucketListSnapshotPeriodForTesting(1); + closeLedger(*app); + + // Update bucketListTargetSizeBytes so bucketListWriteFeeGrowthFactor comes + // into play + auto const& blSize = app->getLedgerManager() + .getSorobanNetworkConfig() + .getAverageBucketListSize(); + { + LedgerTxn ltx(app->getLedgerTxnRoot()); + auto costKey = configSettingKey( + ConfigSettingID::CONFIG_SETTING_CONTRACT_LEDGER_COST_V0); + + auto costEntry = ltx.load(costKey); + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListTargetSizeBytes = blSize * .95; + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListWriteFeeGrowthFactor = 1000; + ltx.commit(); + closeLedger(*app); + } + std::vector txsToSign; auto uploadRes = From ace6d847925a4fa0505a0072db5e004e781247e8 Mon Sep 17 00:00:00 2001 From: Siddharth Suresh Date: Fri, 22 Dec 2023 15:04:11 -0800 Subject: [PATCH 2/4] Add safety check for settings upgrades --- src/main/SettingsUpgradeUtils.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/main/SettingsUpgradeUtils.cpp b/src/main/SettingsUpgradeUtils.cpp index 528545e72f..3aa71b1f30 100644 --- a/src/main/SettingsUpgradeUtils.cpp +++ b/src/main/SettingsUpgradeUtils.cpp @@ -138,12 +138,39 @@ getCreateTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, return {txEnv, contractSourceRefLedgerKey, contractID}; } +void +validateConfigUpgradeSet(ConfigUpgradeSet const& upgradeSet) +{ + // As of 12/22/2023, this is roughly the size of the buckets. This is used + // in a safety check to prevent us from accidentally reducing the target + // size to a point where upgrade becomes prohibitively expensive. We also + // make sure the growth factor has an upper bound here. This file is just + // used by our command line tooling, so we can update this value whenever we + // want. + static const uint64_t CURRENT_LEDGER_SIZE = 12'500'000'000; + for (auto const& entry : upgradeSet.updatedEntry) + { + if (entry.configSettingID() == CONFIG_SETTING_CONTRACT_LEDGER_COST_V0) + { + if (entry.contractLedgerCost().bucketListTargetSizeBytes < + CURRENT_LEDGER_SIZE * .95 || + entry.contractLedgerCost().bucketListWriteFeeGrowthFactor > + 50'000) + { + throw std::runtime_error("Invalid contractLedgerCost"); + } + } + } +} + std::pair getInvokeTx(PublicKey const& publicKey, LedgerKey const& contractCodeLedgerKey, LedgerKey const& contractSourceRefLedgerKey, Hash const& contractID, ConfigUpgradeSet const& upgradeSet, SequenceNumber seqNum) { + validateConfigUpgradeSet(upgradeSet); + TransactionEnvelope txEnv; txEnv.type(ENVELOPE_TYPE_TX); From 6cdf449dc14ed7f106e272d82bf574276ffc25ba Mon Sep 17 00:00:00 2001 From: Siddharth Suresh Date: Fri, 22 Dec 2023 15:58:07 -0800 Subject: [PATCH 3/4] Fix test --- .../test/InvokeHostFunctionTests.cpp | 116 +++++++++++++----- 1 file changed, 86 insertions(+), 30 deletions(-) diff --git a/src/transactions/test/InvokeHostFunctionTests.cpp b/src/transactions/test/InvokeHostFunctionTests.cpp index a5f93290db..81e91f4536 100644 --- a/src/transactions/test/InvokeHostFunctionTests.cpp +++ b/src/transactions/test/InvokeHostFunctionTests.cpp @@ -2485,36 +2485,6 @@ TEST_CASE("settings upgrade command line utils", "[tx][soroban][upgrades]") auto a1 = root.create("A", startingBalance); - // Update the snapshot period and close a ledger to update - // mAverageBucketListSize - app->getLedgerManager() - .getMutableSorobanNetworkConfig() - .setBucketListSnapshotPeriodForTesting(1); - closeLedger(*app); - - // Update bucketListTargetSizeBytes so bucketListWriteFeeGrowthFactor comes - // into play - auto const& blSize = app->getLedgerManager() - .getSorobanNetworkConfig() - .getAverageBucketListSize(); - { - LedgerTxn ltx(app->getLedgerTxnRoot()); - auto costKey = configSettingKey( - ConfigSettingID::CONFIG_SETTING_CONTRACT_LEDGER_COST_V0); - - auto costEntry = ltx.load(costKey); - costEntry.current() - .data.configSetting() - .contractLedgerCost() - .bucketListTargetSizeBytes = blSize * .95; - costEntry.current() - .data.configSetting() - .contractLedgerCost() - .bucketListWriteFeeGrowthFactor = 1000; - ltx.commit(); - closeLedger(*app); - } - std::vector txsToSign; auto uploadRes = @@ -2767,6 +2737,36 @@ TEST_CASE("settings upgrade command line utils", "[tx][soroban][upgrades]") txsToSign.emplace_back(invokeRes.first); auto const& upgradeSetKey = invokeRes.second; + // Update the snapshot period and close a ledger to update + // mAverageBucketListSize + app->getLedgerManager() + .getMutableSorobanNetworkConfig() + .setBucketListSnapshotPeriodForTesting(1); + closeLedger(*app); + + // Update bucketListTargetSizeBytes so bucketListWriteFeeGrowthFactor comes + // into play + auto const& blSize = app->getLedgerManager() + .getSorobanNetworkConfig() + .getAverageBucketListSize(); + { + LedgerTxn ltx(app->getLedgerTxnRoot()); + auto costKey = configSettingKey( + ConfigSettingID::CONFIG_SETTING_CONTRACT_LEDGER_COST_V0); + + auto costEntry = ltx.load(costKey); + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListTargetSizeBytes = blSize * .95; + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListWriteFeeGrowthFactor = 1000; + ltx.commit(); + closeLedger(*app); + } + for (auto& txEnv : txsToSign) { txEnv.v1().signatures.emplace_back(SignatureUtils::sign( @@ -2871,6 +2871,28 @@ TEST_CASE("settings upgrade command line utils", "[tx][soroban][upgrades]") checkSettings(initialEntries); } + // The rest are failure tests, so flip back our cost overrides so we can + // check that the initial settings haven't changed. + { + LedgerTxn ltx(app->getLedgerTxnRoot()); + auto costKey = configSettingKey( + ConfigSettingID::CONFIG_SETTING_CONTRACT_LEDGER_COST_V0); + + auto costEntry = ltx.load(costKey); + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListTargetSizeBytes = + InitialSorobanNetworkConfig::BUCKET_LIST_TARGET_SIZE_BYTES; + costEntry.current() + .data.configSetting() + .contractLedgerCost() + .bucketListWriteFeeGrowthFactor = + InitialSorobanNetworkConfig::BUCKET_LIST_WRITE_FEE_GROWTH_FACTOR; + ltx.commit(); + closeLedger(*app); + } + auto const& lcl = lm.getLastClosedLedgerHeader(); // The only readWrite key in the invoke op is the one that writes the @@ -2946,6 +2968,40 @@ TEST_CASE("settings upgrade command line utils", "[tx][soroban][upgrades]") b.bytes() = upgradeSetBytes; updateBytes(b); } + + SECTION("Invalid ledger cost parameters") + { + auto costEntryIter = + find_if(upgradeSet.updatedEntry.begin(), + upgradeSet.updatedEntry.end(), [](const auto& entry) { + return entry.configSettingID() == + CONFIG_SETTING_CONTRACT_LEDGER_COST_V0; + }); + + SECTION("Invalid bucketListTargetSizeBytes") + { + // 10GB is too low due to the check in validateConfigUpgradeSet + costEntryIter->contractLedgerCost().bucketListTargetSizeBytes = + 10'000'000'000; + REQUIRE_THROWS_AS( + getInvokeTx(a1.getPublicKey(), contractCodeLedgerKey, + contractSourceRefLedgerKey, contractID, upgradeSet, + a1.getLastSequenceNumber() + 3), + std::runtime_error); + } + + SECTION("Invalid bucketListWriteFeeGrowthFactor") + { + // Value is too high due to the check in validateConfigUpgradeSet + costEntryIter->contractLedgerCost().bucketListWriteFeeGrowthFactor = + 50'001; + REQUIRE_THROWS_AS( + getInvokeTx(a1.getPublicKey(), contractCodeLedgerKey, + contractSourceRefLedgerKey, contractID, upgradeSet, + a1.getLastSequenceNumber() + 3), + std::runtime_error); + } + } } TEST_CASE("overly large soroban values are handled gracefully", "[soroban]") From 249e42f9094c5f65a0135d7f6d62f400dedd532a Mon Sep 17 00:00:00 2001 From: Siddharth Suresh Date: Fri, 22 Dec 2023 17:47:44 -0800 Subject: [PATCH 4/4] Update tx meta --- .../InvokeHostFunctionTests.json | 10 +++++++++- .../InvokeHostFunctionTests.json | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/test-tx-meta-baseline-current/InvokeHostFunctionTests.json b/test-tx-meta-baseline-current/InvokeHostFunctionTests.json index 3e7ac655ed..b037844060 100644 --- a/test-tx-meta-baseline-current/InvokeHostFunctionTests.json +++ b/test-tx-meta-baseline-current/InvokeHostFunctionTests.json @@ -474,7 +474,15 @@ "ledger entry size limit enforced" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ], "refund account merged" : [ "bKDF6V5IzTo=", "y0mODi4DFnM=", "o9mb+GfjtK8=", "HG7OrQT/Ofg=" ], "settings upgrade" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ], - "settings upgrade command line utils" : [ "TByXElZpITE=", "TByXElZpITE=", "TByXElZpITE=", "TByXElZpITE=" ], + "settings upgrade command line utils" : + [ + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=" + ], "state archival" : [ "bKDF6V5IzTo=", diff --git a/test-tx-meta-baseline-next/InvokeHostFunctionTests.json b/test-tx-meta-baseline-next/InvokeHostFunctionTests.json index b013bc90f8..08da7fa427 100644 --- a/test-tx-meta-baseline-next/InvokeHostFunctionTests.json +++ b/test-tx-meta-baseline-next/InvokeHostFunctionTests.json @@ -475,7 +475,15 @@ "ledger entry size limit enforced" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ], "refund account merged" : [ "bKDF6V5IzTo=", "y0mODi4DFnM=", "o9mb+GfjtK8=", "HG7OrQT/Ofg=" ], "settings upgrade" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ], - "settings upgrade command line utils" : [ "TByXElZpITE=", "TByXElZpITE=", "TByXElZpITE=", "TByXElZpITE=" ], + "settings upgrade command line utils" : + [ + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=", + "TByXElZpITE=" + ], "state archival" : [ "bKDF6V5IzTo=",