Skip to content
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

runtime-sdk: Implement message emission gas #1662

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 13, 2024

Previously one had to explicitly configure the maximum number of messages to consensus layer that can be emitted by a transaction. This posed a problem for Ethereum-compatible transactions which don't support this field.

This implementation adds message emission gas which is dynamically calculated based on MAX_BATCH_GAS and MAX_MESSAGES. Similar to storage gas, it is only charged in case other use is too small in order to limit the total number of messages that can be emitted in a batch.

All Ethereum-compatible transactions now use this dynamic limit for the maximum number of consensus messages to be emitted.

Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for oasisprotocol-oasis-sdk canceled.

Name Link
🔨 Latest commit bbd26a9
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-oasis-sdk/deploys/65f47f525a738f0008414323

@CedarMist CedarMist self-requested a review March 13, 2024 09:38
@kostko kostko force-pushed the kostko/feature/messages-gas branch from 2516bb0 to 76cb1e8 Compare March 13, 2024 10:19
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 65.28%. Comparing base (e7e0b93) to head (bbd26a9).

Files Patch % Lines
runtime-sdk/src/modules/core/mod.rs 73.33% 4 Missing ⚠️
runtime-sdk/src/modules/consensus/mod.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   65.25%   65.28%   +0.03%     
==========================================
  Files         114      114              
  Lines        8349     8365      +16     
==========================================
+ Hits         5448     5461      +13     
- Misses       2901     2904       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kostko kostko force-pushed the kostko/feature/messages-gas branch 4 times, most recently from 7d08b65 to 04c38eb Compare March 14, 2024 10:44
@CedarMist
Copy link
Member

I've tested this locally with multiple delegations (10 as an example) and it works as expected.

Performing 10 delegations this costs 1171870 gas, so with a 15mm gas limit this allows 128 delegations.

I have a branch in sapphire-paratime which adds a test for this: oasisprotocol/sapphire-paratime#287

Copy link
Member

@CedarMist CedarMist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me, I have not reviewed the code though.

@@ -175,7 +175,8 @@ pub struct Fee {
/// Maximum amount of gas paid for.
#[cbor(optional)]
pub gas: u64,
/// Maximum amount of emitted consensus messages paid for.
/// Maximum amount of emitted consensus messages paid for. Zero means that up to the maximum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use max u32 instead of 0 to indicate that up to the maximum number of per-batch messages can be emitted? Maybe we even need zero to disallow consensus messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using zero makes it easier to use because this field is already deployed and just omitting the field will result in the new behavior be in effect (which makes the most sense for most use cases). There is no reason to actually have this field now, and we can consider removing it in a later iteration.

runtime-sdk/src/modules/core/mod.rs Outdated Show resolved Hide resolved
tests/e2e/simple_consensus.go Outdated Show resolved Hide resolved
Previously one had to explicitly configure the maximum number of
messages to consensus layer that can be emitted by a transaction. This
posed a problem for Ethereum-compatible transactions which don't support
this field.

This implementation adds message emission gas which is dynamically
calculated based on MAX_BATCH_GAS and MAX_MESSAGES. Similar to storage
gas, it is only charged in case other use is too small in order to limit
the total number of messages that can be emitted in a batch.

All Ethereum-compatible transactions now use this dynamic limit for the
maximum number of consensus messages to be emitted.
@kostko kostko force-pushed the kostko/feature/messages-gas branch from 04c38eb to bbd26a9 Compare March 15, 2024 17:03
@kostko kostko enabled auto-merge March 15, 2024 17:10
@kostko kostko merged commit fec2fa9 into main Mar 15, 2024
29 checks passed
@kostko kostko deleted the kostko/feature/messages-gas branch March 15, 2024 17:24
github-actions bot added a commit that referenced this pull request Mar 15, 2024
…ostko/feature/messages-gas

runtime-sdk: Implement message emission gas fec2fa9
github-actions bot added a commit that referenced this pull request Mar 15, 2024
…/kostko/feature/messages-gas

runtime-sdk: Implement message emission gas fec2fa9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants