-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(protocol): PCR-2 fixed base fee #3517
base: develop
Are you sure you want to change the base?
Conversation
a14717f
to
012b96b
Compare
…osts-burned' into protocol/pcr2
fn bucketize_computation(&mut self) -> Result<(), ExecutionError> { | ||
let gas_used = self.gas_status.gas_used_pre_gas_price(); | ||
let gas_used = if let Some(gas_rounding) = self.gas_rounding_step { | ||
if gas_used > 0 && gas_used % gas_rounding == 0 { | ||
gas_used * self.gas_price | ||
} else { | ||
((gas_used / gas_rounding) + 1) * gas_rounding * self.gas_price | ||
let mut computation_units = self.gas_status.gas_used_pre_gas_price(); | ||
if let Some(gas_rounding) = self.gas_rounding_step { | ||
if computation_units == 0 || computation_units % gas_rounding > 0 { | ||
computation_units = ((computation_units / gas_rounding) + 1) * gas_rounding; | ||
} | ||
} else { | ||
let bucket_cost = get_bucket_cost(&self.cost_table.computation_bucket, gas_used); | ||
// charge extra on top of `computation_cost` to make the total computation | ||
// cost a bucket value | ||
bucket_cost * self.gas_price | ||
computation_units = get_bucket_cost(&self.cost_table.computation_bucket, computation_units); | ||
}; | ||
if self.gas_budget <= gas_used { | ||
let computation_cost = computation_units * self.gas_price; | ||
if self.gas_budget <= computation_cost { | ||
self.computation_cost = self.gas_budget; | ||
Err(ExecutionErrorKind::InsufficientGas.into()) | ||
} else { | ||
self.computation_cost = gas_used; | ||
self.computation_cost = computation_cost; | ||
Ok(()) |
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.
@vekkiokonio please review. The logic should be unchanged here, I just wanted to make the distinction between computation units and computation cost (in Nanos) more clear.
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.
LGTM!
This pull request has been deployed to Vercel. Latest commit: ad897c6 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-50ajtfdi2.vercel.app |
This pull request has been deployed to Vercel. Latest commit: ad897c6 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-e473gxuds.vercel.app |
/// remaining gas and storage cost to derive computation cost. | ||
fn summary(&self) -> GasCostSummary { | ||
// compute computation cost burned and storage rebate, both rebate and non refundable fee | ||
let computation_cost_burned = self.computation_cost * self.reference_gas_price / self.gas_price; |
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.
If this is the only change, then we don't need gas_v2. Maybe an if fixed_base_fee
in gas_v1 would be sufficient
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.
@cyberphysic4l pls, check also this!
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 have reverted the addition of gas_v2, and in fact I don't think we even need an if fixed_base_fee
because the computation_cost_burned field is actually unused in protocol v1, so it doesn't matter what we set for this value. So we avoid an additional if statement here.
This pull request has been deployed to Vercel. Latest commit: f6e3013 ✅ Preview: https://apps-ui-kweelww3q-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: f6e3013 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-3d6hav69s.vercel.app |
This pull request has been deployed to Vercel. Latest commit: f6e3013 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-8um4hyami.vercel.app |
This pull request has been deployed to Vercel. Latest commit: f6e3013 ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-3lasjz1fp.vercel.app |
This pull request has been deployed to Vercel. Latest commit: ff78bba ✅ Preview: https://apps-ui-8qik7ktch-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: ff78bba ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-b1h0xflo4.vercel.app |
This pull request has been deployed to Vercel. Latest commit: ff78bba ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-gric8feol.vercel.app |
This pull request has been deployed to Vercel. Latest commit: ff78bba ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-b7tz7ikcc.vercel.app |
]; | ||
let call_arg_arguments = vec![ | ||
CallArg::Pure(bcs::to_bytes(¶ms.computation_charge_burned).unwrap()), |
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.
If it is the only change in the executor we can consider options instead of creating a new execution layer cut, for example:
- Create a new transaction type
EndOfEpochTransactionKind::ChangeEpochV2
. - Or even easier, add a feature flag in the protocol config
advance_epoch_v2
, set it totrue
for the second protocol version, and add thecomputation_charge_burned
parameter if it is enabled.
Creating a cut adds a pretty noticeable amount of code to the repository and should be avoided if it is possible, I believe. Because this increases the repository size and build time.
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 too we can definitively try avoid the use of a new cut!
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.
Ok, if that is the preferred approach I can try one of the suggested alternatives.
This pull request has been deployed to Vercel. Latest commit: 30285b6 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-dkym5ydp6.vercel.app |
This pull request has been deployed to Vercel. Latest commit: 30285b6 ✅ Preview: https://apps-ui-pmt2eihoj-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: 30285b6 ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-hdefaofc7.vercel.app |
This pull request has been deployed to Vercel. Latest commit: 30285b6 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-aloepm8kk.vercel.app |
This pull request has been deployed to Vercel. Latest commit: c2d9200 ✅ Preview: https://apps-ui-mt0fnemg2-iota1.vercel.app |
This pull request has been deployed to Vercel. Latest commit: c2d9200 ✅ Preview: https://apps-backend-29a719c9ccbdb9b80c97b599a599d8953354da-fb0szzkxq.vercel.app |
This pull request has been deployed to Vercel. Latest commit: c2d9200 ✅ Preview: https://iota-rebased-explorer-092603f1de8088243ccb8b861b23e-1kfjg7tnt.vercel.app |
This pull request has been deployed to Vercel. Latest commit: c2d9200 ✅ Preview: https://wallet-dashboard-a225fcfa1e2f852205a61a7cd1d5d93e92-rd8itbhwj.vercel.app |
DON'T REVIEW. THIS IS STILL A DRAFT, JUST RUNNING THE CI.
Description of change
fixed_base_fee
computation_charges_burned
field.advance_epoch
function to burn base fee and pass additional fee to validators.Links to any relevant issues
Fixes #3122
Fixes #4114
Type of change
How the change has been tested
Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.
Change checklist
Tick the boxes that are relevant to your changes, and delete any items that are not.