-
Notifications
You must be signed in to change notification settings - Fork 137
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
implement FIP-0032 gas parameters and charges #534
Conversation
Canonical unit is now milligas everywhere.
// Rules valid for nv16. We will need to be generic over Rules (massive | ||
// generics tax), use &dyn Rules (which breaks other things), or pass | ||
// in the network version, or rules version, to vary these prices going | ||
// forward. |
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.
@Stebalien something to take into account.
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.
In this case, we can likely do &'static dyn Rules
. Or, if we can't statically allocate, Box<dyn Rules>
.
fvm/src/kernel/default.rs
Outdated
.charge_gas(self.call_manager.price_list().on_extern())?; | ||
|
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.
Nit: This repeated pattern makes me think that we should have a capability to either charge for both charges at once or the capability to combine charges into one but it isn't something that needs to get done.
It also shouldn't be consensus critical if we want to add this capability in future.
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.
Agree. I started hacking on a const generics version of charge_gas_n
that took an array and applied all charges sequentially. A few months ago I had also proposed a closure-based version of charge_gas
that took the price list as an argument. That would also remove some pedanticness. However, we can explore these APIs later; right now it doesn't hurt having two explicit charges.
I have no idea why clippy complains here about this and not on master :-/ |
a846563
to
8da9820
Compare
8da9820
to
6208be5
Compare
15f97c9
to
9999204
Compare
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.
Mostly being nitpicky, this looks good.
fvm/src/syscalls/mod.rs
Outdated
/// Applies gas charges upon passing control to the FVM via a syscall. | ||
/// It first applies accumulated execution gas, and then applies the syscall | ||
/// charge for the currently active syscall. | ||
pub fn apply_charges_on_syscall( |
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.
Being nitpicky here, but technically this is a charge on exiting wasm, not just syscalls.
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 a bug. We call this when we return from wasm 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.
Good catch, thanks for that!
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.
Will fix this tomorrow.
// Rules valid for nv16. We will need to be generic over Rules (massive | ||
// generics tax), use &dyn Rules (which breaks other things), or pass | ||
// in the network version, or rules version, to vary these prices going | ||
// forward. |
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.
In this case, we can likely do &'static dyn Rules
. Or, if we can't statically allocate, Box<dyn Rules>
.
fvm/src/syscalls/mod.rs
Outdated
/// Applies gas charges upon passing control to the FVM via a syscall. | ||
/// It first applies accumulated execution gas, and then applies the syscall | ||
/// charge for the currently active syscall. | ||
pub fn apply_charges_on_syscall( |
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 a bug. We call this when we return from wasm as well.
block_open_base: 114617, | ||
block_read_base: 0, | ||
block_open_base: static_milligas!(114617), | ||
block_open_memret_per_byte_cost: 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.
What is memret?
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 memory retention cost (see section under https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0032.md#ipld-state-management-gas).
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.
(Can rename to something else, if it sounds like memory return).
|
||
on_chain_return_value_per_byte: 1, | ||
on_chain_return_value_per_byte: static_milligas!(1), |
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.
Personally, I would set the *_per_byte
storage costs to 1 instead of 1000.
They have an implicit unit of bytes, and then they get multiplied by storage_gas_multiplier
would be milltigas/byte
.
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.
Hm, it's actually implemented the other way around now. The multiplier is no longer denominated in milligas, but as a unitless factor. Are you good with that?
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’m fine with that
2bd8913
to
2bd6769
Compare
Fully implements filecoin-project/FIPs#368 in ref-fvm.
Also changes the unit in pricelists to milligas, and removes now unnecessary functions that dealt with full-unit gas charges.
Adds i64 Gas and Milligas type aliases for extra clarity (but not safety) in APIs.
Static parameters in pricelists were updated to use a non-saturating macro.
Runtime milligas conversions were needed here:
Kernel#charge_gas
accept full Gas for two reasons, to preserve unit homogeneity incharge_*
operations throughout the codebase.Note
Need to review all
on_*
price list formulae because some aren't using saturation arithmetics (inherited). I don't want to continue growing this patch, so we should follow up with this on master.Closes #363.