-
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
Refundable fee updates #3886
Refundable fee updates #3886
Conversation
@@ -603,6 +603,12 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx, | |||
innerResult().code(INVOKE_HOST_FUNCTION_RESOURCE_LIMIT_EXCEEDED); | |||
return false; | |||
} | |||
metrics.mEmitEventByte += out.result_value.data.size(); | |||
if (resources.contractEventsSizeBytes < metrics.mEmitEventByte) |
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 thought contractEventsSizeBytes
is being removed due to your xdr 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.
This PR was created prior to the XDR change, but XDR change only removes the value from resources
. So condition here will need to change to config.txMaxContractEventsSizeBytes
.
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.
Actually, I'll probably integrate the XDR change in this PR to keep things simpler (and also there is something suspicious going on with the fees, I need to investigate that as well). I'll ping this when ready.
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 PR is updated and is ready for review again.
0055fb0
to
2052559
Compare
- Add result size to the events size for fee and limit accounting. - Use the proper error code - Omit redundant tx-specified limit on contract events
@@ -39,6 +39,8 @@ struct MinimumSorobanNetworkConfig | |||
|
|||
static constexpr uint32_t MINIMUM_PERSISTENT_ENTRY_LIFETIME = 4'096; | |||
static constexpr uint32_t MAXIMUM_ENTRY_LIFETIME = 535'680; // 31 days | |||
|
|||
static constexpr uint32_t TX_MAX_CONTRACT_EVENTS_SIZE_BYTES = 200; |
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 the minimum not 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.
Because return value is also counted towards events size and upload wasm and create contract ops return the identifiers. I also added a bit more to be able to get at least some basic events working. I guess minimum could be a bit lower (like 40 bytes), but for initial we'd need a bit more to check that events work.
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 that's right. Maybe we should change the name to reflect both events and results? Either way this doesn't need to block the PR.
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, it would require an XDR change and honestly I'm not sure what would be a better name (contractEventsAndReturnValue
sounds way too clunky IMO)
r+ b74baf8 |
Description
Resolves stellar/rs-soroban-env#995
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)