-
Notifications
You must be signed in to change notification settings - Fork 983
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
Include all upgradeable settings in the upgrade #3888
Conversation
src/ledger/NetworkConfig.h
Outdated
|
||
static constexpr uint32_t TX_MAX_WRITE_LEDGER_ENTRIES = 2; | ||
static constexpr uint32_t TX_MAX_WRITE_BYTES = 2'000; | ||
static constexpr uint32_t TX_MAX_WRITE_BYTES = 3'000; |
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 is actually not consistent with the entry and contract sizes. I think the following should be true:
TX_MAX_READ_BYTES == MAX_CONTRACT_DATA_KEY_SIZE_BYTES + MAX_CONTRACT_DATA_ENTRY_SIZE_BYTES && MAX_CONTRACT_SIZE == MAX_CONTRACT_DATA_ENTRY_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.
FWIW we wanted MAX_CONTRACT_SIZE
is kind of redundant; for v1 we decided to keep it consistent with the contract data entry 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.
Hmm good point. Shouldn't TX_MAX_WRITE_BYTES == MAX_CONTRACT_DATA_KEY_SIZE_BYTES + MAX_CONTRACT_DATA_ENTRY_SIZE_BYTES
be true as well?
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, that would make sense as well. Maybe these should also be the validation conditions (with >=). These are not as critical to get right, but would be a bit annoying if messed up.
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 updated the settings. I don't think we need validation for this, but don't feel strongly so I can add it if you think we should.
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 this is a bit of a footgun, so I would prefer to have a validation on this. It's ok to do in a separate PR though.
r+ de3feb4 |
Description
TX_MAX_INSTRUCTIONS
so only 1 vm can be instantiated. The min is 2,000,000, and the upgrade contract test currently uses 1476437.Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)