Conversation
Co-authored-by: Eric Tu <6364934+ec2@users.noreply.github.com> Co-authored-by: Mikers <snissn@gmail.com> Co-authored-by: raulk <raul@protocol.ai> Co-authored-by: Shashank Trivedi <100513286+lordshashank@users.noreply.github.com>
…pc into simple-genesis
Cargo.toml
Outdated
| "fendermint/vm/*", | ||
| "fendermint/actors", | ||
| "fendermint/actors/chainmetadata", | ||
| "fendermint/actors/gas", |
There was a problem hiding this comment.
Would call this gas_market since gas is too generic and it could refer to, e.g. the gas limit in a message.
fendermint/actors/gas/src/lib.rs
Outdated
| fn invoke_method<RT>( | ||
| rt: &RT, | ||
| method: MethodNum, | ||
| params: Option<IpldBlock>, | ||
| ) -> Result<Option<IpldBlock>, ActorError> | ||
| where | ||
| RT: Runtime, | ||
| RT::Blockstore: Blockstore + Clone, | ||
| { | ||
| if method == Method::Constructor as u64 { | ||
| fil_actors_runtime::dispatch(rt, method, Self::constructor, params) | ||
| } else if method == Method::SetBlockGasLimit as u64 { | ||
| fil_actors_runtime::dispatch(rt, method, Self::set_block_gas_limit, params) | ||
| } else { | ||
| Err(ActorError::not_found("method not found".into())) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we not use the dispatch macros? They weren't practical for our EAM because we needed to override/intercept, but they should be usable here.
| // Copyright 2022-2024 Protocol Labs | ||
| // SPDX-License-Identifier: Apache-2.0, MIT | ||
|
|
||
| define_id!(GAS { id: 66 }); |
There was a problem hiding this comment.
Why this number? I think it would be safer to start backwards from f099.
| /// The total gas available to be used by transactions in the current block | ||
| /// The executor requires Send + Sync, using an atomic variable instead of u64. | ||
| block_gas_quota: AtomicGas, | ||
| p: PhantomData<DB>, |
There was a problem hiding this comment.
| p: PhantomData<DB>, | |
| _p: PhantomData<DB>, |
fendermint/actors/gas/src/lib.rs
Outdated
| if rt.message().caller() != SYSTEM_ACTOR_ADDR { | ||
| return Err(ActorError::forbidden("not system actor".into())); | ||
| } |
There was a problem hiding this comment.
There should be a utility function in the Runtime: validate_immediate_caller or something like that.
| pub(crate) fn read_actor_state<State: DeserializeOwned, DB: Blockstore + Clone + 'static>( | ||
| state: &FvmExecState<DB>, | ||
| actor_id: ActorID, | ||
| ) -> anyhow::Result<State> { | ||
| let state_tree = state.state_tree(); | ||
|
|
||
| let state_cid = state_tree | ||
| .get_actor(actor_id)? | ||
| .ok_or_else(|| anyhow::anyhow!("actor state not found: {}", actor_id))? | ||
| .state; | ||
|
|
||
| Ok(state_tree | ||
| .store() | ||
| .get_cbor::<State>(&state_cid)? | ||
| .ok_or_else(|| anyhow::anyhow!("actor state should not be null"))?) | ||
| } |
There was a problem hiding this comment.
Nice, this will come in handy!
| fn block_gas_limit(&self, state: &Self::State) -> anyhow::Result<Gas> { | ||
| let s = | ||
| read_actor_state::<State, DB>(state, fendermint_vm_actor_interface::gas::GAS_ACTOR_ID)?; | ||
| Ok(s.block_gas_limit()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This shouldn't be accessing the state tree directly, but instead invoking a getter method on the actor for better encapsulation.
| pub(crate) mod default; | ||
|
|
||
| /// Handles the gas modeling in the current blockchain | ||
| pub trait GasLayer { |
There was a problem hiding this comment.
- Let's call this
GasMarket. - Let's document that during normal operation, it's supposed to be backed by an actor.
- A more future-proof, pure interface for this:
pub trait GasMarket<State, Input> {
/// Fetches the current state of the gas market.
fn current() -> State;
/// Updates the state of the gas market from the aggregated gas utilization stats for the block.
fn update(Input) -> State;
}
struct State {
block_gas_limit: Gas,
/// This is the base_fee, but termed in a more abstract way so our terminology is not overfitted to EIP-1559.
burn_fee_per_gas: TokenAmount,
}There was a problem hiding this comment.
Sounds good, then the actor is really simple, just writing the state and reading the state.
| // Block height (FVM epoch) as sequence is intentional | ||
| let height = state.block_height(); | ||
|
|
||
| self.gas.reset_block_gas_quota(&mut state)?; |
There was a problem hiding this comment.
Why do we need a reset? We should be loading the current gas market state from the actor before we begin execution?
There was a problem hiding this comment.
Yep, ideally it's like this. I tried doing this as well, but the current ABCI trait is blocking me from doing this because it's begin(&self, ...). If we reset everytime, then we have to do:
self.gas = GasMarket::try_from(&mut state)?;which requires begin(&mut self).
I tried not to update so many changes and keep the core trait untouched.
fendermint/actors/gas/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl ActorCode for IPCGasActor { |
There was a problem hiding this comment.
I'd call this EIP1559GasMarketActor if we're putting the EIP-1559 logic on chain.
We should define a common interface for gas market actors so that users can implement alternative gas markets and plug them in in their genesis.
|
@raulk I have updated for most of the review feedbacks. Some key changes:
pub trait GasMarket<State, Input> {
/// Updates the state of the gas market from the aggregated gas utilization stats for the block.
fn update(Input) -> State;
} |
Co-authored-by: Karel Moravec <moravec.wdd@gmail.com>
Co-authored-by: Karel Moravec <moravec.wdd@gmail.com>
…pc into simple-genesis
|
|
||
| /// The gas market based on EIP1155 | ||
| #[derive(Default)] | ||
| pub struct EIP1559GasMarket { |
There was a problem hiding this comment.
The actual gas strategy (EIP1559) should be fully encapsulated in the actor, so this is just an ActorGasMarket (a gas market driven by an on-chain actor), i.e. a gas market that resorts to an on-chain actor to keep its state and oscillate the gas parameters.
| impl GasMarket for EIP1559GasMarket { | ||
| type State = EIP1559GasState; | ||
|
|
||
| fn reload_from_chain<DB: Blockstore + Clone + 'static>( |
There was a problem hiding this comment.
I moved load out of the trait. This method is only needed because of the &self trait bound, ideally the gas market should be recreated in begin. GasMarket does not really need to be aware of self loading from state. I shifted to the implementation of ActorGasMarket.
| &self, | ||
| chain_state: &FvmExecState<DB>, | ||
| ) -> anyhow::Result<()> { | ||
| let state = get_state(chain_state)?; |
There was a problem hiding this comment.
We need a query method in the GasMarket actor to fetch the current gas parameters (I called this current() in my proposal above), we should not dive into state directly as that is leaky.
There was a problem hiding this comment.
I added the method to the gas actor and query using the getter method.
But in fendermint I think we dont really have to add the method to GasMarket because it limits the trait to have a State parameter, which GasMarket does not really need to be on chain. I think it's nicer to just keep this as an implementation detail.
| fn update_block_gas_used(&self, old_used: Gas, new_used: Gas) -> anyhow::Result<()> { | ||
| self.block_gas_used | ||
| .compare_exchange(old_used, new_used, Ordering::SeqCst, Ordering::SeqCst) | ||
| .map_err(|_| anyhow!("concurrent update in block gas used, should not happen"))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Why do we need this dance between consume_gas and update_block_gas_used?
There was a problem hiding this comment.
good point, I was trying to be "correct" when using Atomic, but actually there is no need, a direct store will do. Updated.
| /// The block gas limit | ||
| block_gas_limit: AtomicGas, | ||
| /// The accumulated gas usage so far | ||
| block_gas_used: AtomicGas, |
There was a problem hiding this comment.
Is AtomicGas needed purely due to the ownership issues you pointed out on in our last sync? If so, please add a note here + a TODO, since this component is single-threaded and we should attempt to remove this overhead.
There was a problem hiding this comment.
Purely because of the trait bound, I will add the note and todo
| /// The gas market state | ||
| type State; |
There was a problem hiding this comment.
So the more I think about it, I think this trait should be state-type agnostic. The GasMarket should just have its own standard data model with its own struct, and the GasMarket implementation should adapt the concrete gas policy's details to that standard data model / interface.
There was a problem hiding this comment.
Is it possible for the trait implementor to keep a reference to FvmExecState instead of having to pass it in? I think you can have a new(&FvmExecState) trait method, with the lifetime of the GasMarket conditioned to that of the exec state?
There was a problem hiding this comment.
Or maybe it's not possible without an Rc, not sure.
There was a problem hiding this comment.
I have removed the State in GasMarket, it should be state type agnostic, technically no need to know about state and unnecessary.
I tried holding the &mut FvmExecState does not seem doable, because FvmExecState<DB> is destructed after each deliver_tx, then the GasMarket needs to be persisted every txn, which seems unnecessary.
Technically there is no need to be aware of the ChainState for gas, but I also think this is part of a ExecutionContext or something like that to help determine the gas, implementation could just ignore.
There was a problem hiding this comment.
I tried holding the
&mut FvmExecStatedoes not seem doable, becauseFvmExecState<DB>is destructed after eachdeliver_tx, then theGasMarketneeds to be persisted every txn, which seems unnecessary.
Are you sure? FvmExecState seems to be maintained throughout deliver_tx, unless there's some nuance I'm unaware of? Did you try to make gas_market a member of FvmExecState instead of FvmMessageInterpreter?
raulk
left a comment
There was a problem hiding this comment.
This is on the right path, but needs a bit more work before we can land it. A few things missing in this PR. Some are covered by your TODOs in the PR description.
- Block validation logic (ProcessProposal): validate that the sum of transaction gas limits does not exceed the current block gas limit.
- Message validation logic (CheckTx): check that the gas limit does not exceed the block gas limit.
- Genesis: set up the GasMarketActor on genesis and inform CometBFT of the block gas limit during genesis.
- Block Gas Limit updates: inform CometBFT of block gas limit changes in
end_block, so that it binpacks transactions to fit within the block gas limit. - Tests: what do you have in mind here?
- Docs: explain how to configure the default (EIP1559) gas market on genesis, how to update it, and how to replace it with a custom gas market.
| /// Base fee max change denominator as defined in [EIP-1559](https://eips.ethereum.org/EIPS/eip-1559) | ||
| const EIP1559_BASE_FEE_MAX_CHANGE_DENOMINATOR: u64 = 8; | ||
| /// Elasticity multiplier as defined in [EIP-1559](https://eips.ethereum.org/EIPS/eip-1559) | ||
| const EIP1559_ELASTICITY_MULTIPLIER: u64 = 2; | ||
| /// Initial base fee as defined in [EIP-1559](https://eips.ethereum.org/EIPS/eip-1559) | ||
| pub const EIP1559_INITIAL_BASE_FEE: u64 = 1_000_000_000; |
There was a problem hiding this comment.
Ideally we'd load these parameters from the environment, so we can adjust them without having to deploy new actor code. Alternatively, we could make them state variables, set in the constructor, and updatable through an admin method.
Let's open an issue and track this customisation separately, not worth doing now.
There was a problem hiding this comment.
We should call this MINIMUM_BASE_FEE, and start with that base fee on upgraded networks.
| Constructor = METHOD_CONSTRUCTOR, | ||
| GetState = frc42_dispatch::method_hash!("GetState"), | ||
| SetBlockGasLimit = frc42_dispatch::method_hash!("SetBlockGasLimit"), | ||
| UpdateBlockGasConsumption = frc42_dispatch::method_hash!("UpdateBlockGasConsumption"), |
There was a problem hiding this comment.
- Let's broaden this name up to
UpdateUtilization. - Let's make it take a struct for better extensibility. In the future, we may want to oscillate gas based on other stats, e.g. number of unique senders, gas density of transactions, gas usage distribution, etc. And it's very impractical to have single methods for each element.
| pub enum Method { | ||
| Constructor = METHOD_CONSTRUCTOR, | ||
| GetState = frc42_dispatch::method_hash!("GetState"), | ||
| SetBlockGasLimit = frc42_dispatch::method_hash!("SetBlockGasLimit"), |
There was a problem hiding this comment.
Same as below, let's make widen the name to SetConstants. Today we have the block gas limit only, but we may want to pass in other constants like the consts above, so let's make it take a struct.
Note that these methods will be generic for all gas markets, so we shouldn't overfit this interface to EIP-1559 semantics.
| #[repr(u64)] | ||
| pub enum Method { | ||
| Constructor = METHOD_CONSTRUCTOR, | ||
| GetState = frc42_dispatch::method_hash!("GetState"), |
There was a problem hiding this comment.
GetState is inaccurate and leaky: it conveys that the values returned are read strictly from the state. It might not be the case, e.g. other gas markets may perform computation that doesn't depend on the actor state, e.g. epoch numbers, randomness, calling other actors, etc.
Suggestion:
- Name the method
ReadCurrent. - And the struct
GasMarketReading.
So we model "readings" of the gas market.
| fn update_base_fee(gas_limit: Gas, gas_used: Gas, base_fee: TokenAmount) -> TokenAmount { | ||
| let gas_target = gas_limit / EIP1559_ELASTICITY_MULTIPLIER; | ||
|
|
||
| if gas_used == gas_target { | ||
| return base_fee; | ||
| } | ||
|
|
||
| if gas_used > gas_target { | ||
| let gas_used_delta = gas_used - gas_target; | ||
| let base_fee_delta = base_fee | ||
| .clone() | ||
| .mul(gas_used_delta) | ||
| .div_floor(gas_target) | ||
| .div_floor(EIP1559_BASE_FEE_MAX_CHANGE_DENOMINATOR) | ||
| .max(TokenAmount::from_atto(1)); | ||
| base_fee + base_fee_delta | ||
| } else { | ||
| let gas_used_delta = gas_target - gas_used; | ||
| let base_fee_per_gas_delta = base_fee | ||
| .clone() | ||
| .mul(gas_used_delta) | ||
| .div_floor(gas_target) | ||
| .div_floor(EIP1559_BASE_FEE_MAX_CHANGE_DENOMINATOR); | ||
| if base_fee_per_gas_delta > base_fee { | ||
| TokenAmount::zero() | ||
| } else { | ||
| base_fee - base_fee_per_gas_delta | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Where did you take this logic from? Please link to the original source.
There was a problem hiding this comment.
All of this code block can be made less pedantic / more idiomatic, but let's get these changes in before we do a cleanup.
| if block_gas_used + gas >= self.block_gas_limit.load(Ordering::SeqCst) { | ||
| anyhow::bail!("out of block gas") | ||
| } |
There was a problem hiding this comment.
This isn't the right place to check this; it can serve as a sanity check here (panic level), but not as the primary site where we check this.
There was a problem hiding this comment.
I captured this error outside and log it as warning.
| // TODO: maybe compare the gas limits is better? | ||
| msg.gas_limit = msg.gas_limit.min(self.gas.available_block_gas()); | ||
|
|
There was a problem hiding this comment.
This is better modelled as a pre_exec and a post_exec hook. If the gas limit exceeds the available gas, we should abort, not cap the execution gas. It is the block proposer's mistake, not the user's mistake (if you do this, you'd likely provoke an out of gas despite them having set the gas limit correctly).
| (apply_ret, emitters, latency) | ||
| } else { | ||
| // TODO: maybe compare the gas limits is better? | ||
| msg.gas_limit = msg.gas_limit.min(self.gas.available_block_gas()); |
There was a problem hiding this comment.
This is incorrect here. Instead, we should validate that the sum of all transaction gas limits doesn't exceed the block limit, at block validation time prepare_proposal. That guarantees we have enough gas to process all transactions even if they squeeze their gas limits to the max. Then this situation cannot occur.
| let (execution_result, latency) = measure_time(|| state.execute_explicit(msg.clone())); | ||
| let (apply_ret, emitters) = execution_result?; | ||
|
|
||
| self.gas.consume_gas(apply_ret.msg_receipt.gas_used)?; |
There was a problem hiding this comment.
| self.gas.consume_gas(apply_ret.msg_receipt.gas_used)?; | |
| self.gas.record_gas_used(apply_ret.msg_receipt.gas_used)?; |
| // Block height (FVM epoch) as sequence is intentional | ||
| let height = state.block_height(); | ||
|
|
||
| self.gas.load(&mut state)?; |
There was a problem hiding this comment.
| self.gas.load(&mut state)?; | |
| self.gas_market.load(&mut state)?; |
Co-authored-by: raulk <raul@protocol.ai>
…pc into simple-genesis
Co-authored-by: cryptoAtwill <willes.lau@protocol.ai> Co-authored-by: raulk <raul@protocol.ai>
Co-authored-by: cryptoAtwill <willes.lau@protocol.ai>
| } | ||
|
|
||
| #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone)] | ||
| pub struct SetConstants { |
There was a problem hiding this comment.
| pub struct SetConstants { | |
| pub struct Constants { |
|
|
||
| impl EIP1559GasMarketActor { | ||
| /// Creates the actor | ||
| pub fn constructor(rt: &impl Runtime, st: EIP1559GasState) -> Result<(), ActorError> { |
There was a problem hiding this comment.
This should take a ConstructorParams object, otherwise you're leaking the state object externally, which is highly undesirable as it breaks abstraction.
struct ConstructorParams {
constants: Constants,
initial_base_fee: TokenAmunt,
}| fn set_constants(rt: &impl Runtime, constants: SetConstants) -> Result<(), ActorError> { | ||
| rt.validate_immediate_caller_is(std::iter::once(&SYSTEM_ACTOR_ADDR))?; | ||
|
|
||
| rt.transaction(|st: &mut EIP1559GasState, _rt| { | ||
| st.block_gas_limit = constants.block_gas_limit; | ||
| Ok(()) | ||
| })?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
We should also have a get_constants that returns the constants, invokable by any.
There was a problem hiding this comment.
I know this has been open for a while, but it needs a rebase!
Co-authored-by: raulk <raul@protocol.ai>
As part of the #925, this PR only adds block gas limit.
The following changes are added:
IPCGasActorthat tracks the gas related parameters in a separate actor.GasLayertrait that summaries the key gas functionalities.prepareandprocess.