-
Notifications
You must be signed in to change notification settings - Fork 970
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.
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
Reduce the default resource limits to minimum. #3885
Conversation
@@ -197,7 +197,8 @@ TransactionFrame::getResources() const | |||
|
|||
return Resource({opCount, r.instructions, txSize, r.readBytes, | |||
r.writeBytes, | |||
static_cast<int64_t>(r.footprint.readOnly.size()), | |||
static_cast<int64_t>(r.footprint.readOnly.size() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
@@ -1192,7 +1196,7 @@ TEST_CASE("Soroban TransactionQueue limits", | |||
{ | |||
auto tx2 = createUploadWasmTx( | |||
*app, account2, initialFee * 2, refundableFee, resources, | |||
std::nullopt, /* addInvalidOps */ 99); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the threshold is just 1 op, so adding 99 ops arguably provides worse coverage (the test would pass if threshold was 98)
- 99 large ops caused the transaction to be too large, so it covered the wrong code path
src/overlay/test/OverlayTests.cpp
Outdated
#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION | ||
overrideSorobanNetworkConfigForTest(*app1); | ||
overrideSorobanNetworkConfigForTest(*app2); | ||
modifySorobanNetworkConfig(*app1, [txSize](SorobanNetworkConfig& cfg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extract the lambda to avoid duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
res = txtest::makeConfigUpgradeSet(ltx, configUpgradeSet); | ||
ltx.commit(); | ||
} | ||
txtest::executeUpgrade(*app, txtest::makeConfigUpgrade(*res)); | ||
}; | ||
upgradeApp(app1, txSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed if the config is overridden on line 163?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably due to ledger-wide limit... Also this section concerns upgrades, so it would make sense to do the upgrade for the setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section does upgrades in a controlled way, where order and delay between upgrades matters (and upgrades trigger overlay events as well). Why not just set ledger-wide limit in the same lambda you're using above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config overrides just override the config in-memory. This is a test util sufficient for most of the tests that don't care about the ledger state. As upgrades in fact do care about ledger state, I feel that it's more appropriate to prepare the proper ledger state first via the proper (upgrade) mechanism.
Also refactor the test framework to have only a single way of modifying the config in tests (besides upgrades).
r+ ef847f0 |
Description
Reduce the default resource limits to minimum.
Also refactor the test framework to have only a single way of modifying the config in tests (besides upgrades).
Even if we decide to go with higher limits initially, this change makes it as easy to update the limits without breaking tests.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)