-
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
Setting validations #3855
Setting validations #3855
Conversation
Once we settle on the initial values, I'll update the LedgerEntryIsValid invariants as well. |
src/herder/Upgrades.cpp
Outdated
valid = | ||
cfg.stateExpirationSettings().maxEntryExpiration > 0 && | ||
cfg.stateExpirationSettings().minTempEntryExpiration > 0 && | ||
cfg.stateExpirationSettings().minPersistentEntryExpiration > |
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.
I think we need a lower bound on max entry expiration in order to make sure that we can bump the upgrade entry reasonably (a few days/weeks?). Also probably need some small threshold on min persistent expiration, like 100 ledgers or something, so that there is time to bump it.
src/herder/Upgrades.cpp
Outdated
case ConfigSettingID::CONFIG_SETTING_CONTRACT_META_DATA_V0: | ||
valid = cfg.contractMetaData().txMaxExtendedMetaDataSizeBytes > | ||
MinimumSorobanNetworkConfig::MIN_WRITE_BYTES && |
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.
Hmm, this should actually be greater than the write size in the current/updated config. Also the lower bound threshold should probably be set separately to something like 2-2.5x of MIN_WRITE_BYTES
.
upgradeApp(app1, | ||
MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES); | ||
upgradeApp(app2, | ||
MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES); |
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.
I think we should resize tx1 as well: the idea in this section is that we decrease max soroban tx size through an upgrade, but still allow tx1 to go through because we pick the limit as max(classic_tx_limit, soroban_tx_limit). So tx1 should be greater than MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES but within the classic limit (whereas tx2 is greater than both limits, and therefore, can't go through)
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.
I resized tx1 and pushed, but after reading your comment, I don't think I did this correctly. Looking over it now.
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.
So I think the limit decrease should be to txSize
instead of MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES
, but the test still passes when I decrease to MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES
. How is tx1 able to go through?
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.
tx1 can go through either way: if soroban limit is too small, it still works because classic limit is enough (it's set to tx1 size in this test). if soroban limit is large (like the one you set) it doesn't really matter, since both limits are high enough for tx1. The point of this section is to verify that overlay correctly allows tx1 to go through despite soroban limit being too low (since it picks classic limit instead). So I think you need to resize tx1 at the beginning of the test to be greater than MinimumSorobanNetworkConfig::TX_MAX_SIZE_BYTES but less than tx2, then the rest of the test should work as-is.
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.
Ah I see. Then the test should be good now.
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.
thanks! looks good to me
src/invariant/LedgerEntryIsValid.cpp
Outdated
cfg.contractLedgerCost().bucketListTargetSizeBytes <= 0 || | ||
cfg.contractLedgerCost().writeFee1KBBucketListLow <= 0 || | ||
cfg.contractLedgerCost().writeFee1KBBucketListHigh <= 0 || | ||
cfg.contractLedgerCost().bucketListWriteFeeGrowthFactor > 0) |
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 looks incorrect (should be <0?)
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.
Yeah I committed the invariant change prematurely. Fixing now.
bc9631d
to
8a86ff2
Compare
r+ 8a86ff2 |
Description
Resolves #3802.
I chose some arbitrary min values that should work with an upgrade contract, but I still need to test against a standalone network. Putting this PR out there to get some eyes on it.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)