feat(node): prepare + process + end block abci gas handling #1122

cryptoAtwill · 2024-08-22T12:36:19Z

This PR add gas handling to prepare, process and end_block abci calls. Some key changes:

prepare: Prepare the proposal with gas limit check. From the doc, cometbft will be able to ensure the block_gas_limit is enforce (that's what I inferred from the doc, somehow not 100% clearly), but still added as a sanity check.
process: Same as prepare, checks the block gas limit.
Added SetConstants into the gas market trait. Currently they are not used, added allow(dead_code) for future usage. The idea is SetConstants will buffer the update and in end block will commit the changes. Before each end_block finishes, it will check if there are SetConstants called and if there is, it will fetch latest consensus params from cometbft, update accordingly and push to cometbft.

Update:

Shifted tendermint import from interpreters to App so that interpreters are more tendermint agnostic.
Added ValidatorTracker and Client to App so that communication with cometbft can be made to query consensus parameters.
Replaced tendermint::account::Id from FvmExecState with PublicKey. The id is the first 20 bytes of public key hashed, which is not useful for deriving fvm addresses. Converting to PublicKey will make the downstream process much more helpful.
Added MessageSelector trait to handle more generic message selection.

cryptoAtwill · 2024-08-22T12:41:23Z

fendermint/vm/interpreter/src/chain.rs

+    let mut msgs_with_gas_limit = signed_msgs_with_gas_limit(msgs)?;
+
+    // sort by gas limit descending
+    msgs_with_gas_limit.sort_by(|a, b| b.1.cmp(&a.1));


probably we dont need this or handle if block gas limit is hit.

raulk · 2024-08-24T10:37:25Z

fendermint/app/src/app.rs

+        let (state_params, block_height) =
+            self.state_params_at_height(request.height.value().into())?;
+        let state = FvmExecState::new(
+            self.state_store_clone(),
+            self.multi_engine.as_ref(),
+            block_height as ChainEpoch,
+            state_params,
+        )
+        .context("error creating new state")?;
+


Is it not possible to reuse new_read_only_exec_state()?

Don't we have the guarantee that this runs always against the latest committed state, therefore we could simply use that state, instead of loading it from the height that's passed in?

raulk · 2024-08-24T10:42:20Z

fendermint/app/src/app.rs

+        // TODO: This seems leaky placed here, should be done in `interpreter`, that's where it's ipc
+        // TODO: aware, here might have filtered more important messages.
        let (txs, size) = take_until_max_size(txs, request.max_tx_bytes.try_into().unwrap());


Could you elaborate on this? Is the claim here that if we're hitting the limit, we might end up pruning IPC system messages? (e.g. checkpoints, top-down finality)? Would you rather pass the block byte limit to the interpreter, so it can prioritise IPC system messages? Generally speaking, that makes sense to me. The added benefit of that refactor is that we'd be making message packing consensus-engine-agnostic.

For example, if you checkout

ipc/fendermint/vm/interpreter/src/chain.rs

Line 166 in f4c6748

msgs.push(ChainMessage::Ipc(IpcMessage::TopDownExec(ParentFinality {

ipc critical messages are appended to the end of the list. If there are quite some messages, the ipc messages might be filtered out as messages are selected from the start.

Since we are introduce some form of message selection, I was looking at some way to put all the selection logic in one place instead of split into different parts of the codebase. I think passing the block limit into the interpreter works for me, at least we have message selection in one place, easier to debug, onboard and maintain.

raulk · 2024-08-24T10:42:48Z

fendermint/app/src/app.rs

+        let (state_params, block_height) =
+            self.state_params_at_height(request.height.value().into())?;
+        let state = FvmExecState::new(
+            self.state_store_clone(),
+            self.multi_engine.as_ref(),
+            block_height as ChainEpoch,
+            state_params,
+        )
+        .context("error creating new state")?;
+


Same as above. If this runs on the committed state, we can just use the helper?

raulk · 2024-08-24T10:44:06Z

fendermint/vm/interpreter/src/chain.rs

+                let gas_limit = inner.message.gas_limit;
+                Ok((ChainMessage::Signed(inner), gas_limit))
+            }
+            ChainMessage::Ipc(_) => Err(anyhow!("should not have ipc messages in user proposals")),


Is this error elevated to a panic?

This depends on how caller handle error. From the implementation, it will cause panic. ipc message should never be proposed, if proposed then it means the validator is doing suspicious stuff. We can give a warn instead and continue processing. Not sure if anyone will pick up in logs/monitoring.

raulk · 2024-08-24T10:51:06Z

fendermint/vm/interpreter/src/chain.rs

+/// Performs message selection:
+/// - Order by gas limit in descending order
+/// - Make sure total gas limit does not exceed the `total_gas_limit` parameter
+fn messages_selection(


From what I gather, CometBFT should always be staying under the block gas limit when selecting messages. We are informing gas_wanted in CheckTx, and now CometBFT will know about the block_gas_limit. Is this intended to be an extra filter just in case CometBFT messed up?

Note that we can also set a priority in CheckTx, so we could prioritise checkpoint operations, deposits, withdrawals, staking, unstaking, etc. simply by informing CometBFT of a higher priority.

Generally I think we're duplicating work here, since we can get CometBFT to handle much of this for us (even though that makes us less consensus-agnostic for the time being).

Yeah, I also have this impression that cometbft will handle it, but I could not fully deduce that in https://docs.cometbft.com/v0.37/spec/abci/abci++_app_requirements#gas. So I added it anyways. I can remove it for now.

However I doubt CheckTx with priority from cometbft will work. The checkpoint operations and etc are derived completely from fendermint, they are not coming from cometbft. We have to manually put them at the head of the msgs.

raulk · 2024-08-24T11:06:43Z

fendermint/vm/interpreter/src/fvm/gas/actor.rs

+    type Constant = SetConstants;
+
+    fn get_constants(&self) -> anyhow::Result<Self::Constant> {
+        todo!()
+    }
+
+    fn set_constants(&mut self, constants: Self::Constant) {
+        self.constant_update = Some(constants);
+    }
+


Are GetConstants and SetConstants ever used by the App? These are policy-specific methods for the user to invoke off band.

Yep, they are not used by the App and called by system upgrade, maybe.

I added these two for "reference" implementation I guess. The SetConstants is also related to how state could detect potential gas market changes. I'm thinking of holding the changes in memory first, as reflected in the option field, then in end_block performs the commit in interpreter. Then send a summary of the changed to App which then can be propagated to cometbft. If we dont control SetConstants, we might lost the change or have to query the gas market over and over to detect changes.

raulk · 2024-08-24T11:16:35Z

fendermint/vm/interpreter/src/fvm/gas/actor.rs

+    fn commit_constants<E: Executor>(
+        &self,
+        executor: &mut E,
+        block_height: ChainEpoch,
+    ) -> anyhow::Result<()> {
+        let Some(ref constants) = self.constant_update else {
+            return Ok(());
+        };
+
+        let msg = FvmMessage {
+            from: system::SYSTEM_ACTOR_ADDR,
+            to: GAS_MARKET_ACTOR_ADDR,
+            sequence: block_height as u64,
+            // exclude this from gas restriction
+            gas_limit: i64::MAX as u64,
+            method_num: fendermint_actor_gas_market::Method::SetConstants as u64,
+            params: fvm_ipld_encoding::RawBytes::serialize(constants)?,
+            value: Default::default(),
+            version: Default::default(),
+            gas_fee_cap: Default::default(),
+            gas_premium: Default::default(),
+        };
+        self.call_fvm(msg, executor)?;
+
+        Ok(())
+    }


We don't need to handle constant changes at all, these happen entirely as standard chain messages, right? Or that's what I thought?

The only thing we need to be careful with is not changing constants in the middle of block execution. So I think we'll need a message selection / prepare proposal / process proposal rule to enforce messages directed to the gas market actor to always be at the top of the block.

Does that make sense?

So given a data structure:

struct MessageOrderRule { from: Option<Address>, to: Option<Address>, precedence: i32, }

We could have a lazy static:

lazy_static! { pub static ref MESSAGE_ORDER_POLICY: Vec<MessageOrderRule> = vec![ MessageOrderRule{from: None, to: Some(Address::from_id(GAS_MARKET_ID), precedence: 1000}, }

And then we'd reoder messages during selection to satisfy the policy?

Actually, we could have this policy be a trait with methods to sort (prepare proposal) and validate order (process proposal).

I think a trait is better, more general and flexible. Just holding a vec of MessageOrderRule traits implementations.

raulk · 2024-08-24T11:17:10Z

fendermint/vm/interpreter/src/fvm/gas/actor.rs

@@ -96,13 +152,18 @@ impl ActorGasMarket {
            gas_premium: Default::default(),
        };

+        self.call_fvm(msg, executor)?;


Suggested change

self.call_fvm(msg, executor)?;

self.apply_implicit_message(msg, executor)?;

raulk · 2024-08-24T11:19:47Z

fendermint/vm/interpreter/src/fvm/gas/actor.rs

        if let Some(err) = apply_ret.failure_info {
            anyhow::bail!("failed to update EIP1559 gas state: {}", err)
        } else {
-            Ok(())
+            Ok(apply_ret)
        }
    }


Can be simplified (GitHub won't let me make a suggestion):

apply_ret.failure_info .map_or(Ok(apply_ret), |err| anyhow::bail!("failed to update EIP1559 gas state: {}", err))

I tried, seems the above does not compile due to partial move because map_or takes self which consumes apply_ret.failure_info and Ok(apply_ret) is partially moved. I tried the same with as_ref, but have the same issue. It works if I do apply_ret.failure_info.take, but perhaps not desired as it changes the failure info.

raulk · 2024-08-24T11:20:04Z

fendermint/vm/interpreter/src/fvm/gas/mod.rs

+    /// The constant parameters that determines the readings of gas market, such as block gas limit.
+    type Constant;
+
+    #[allow(dead_code)]
+    fn get_constants(&self) -> anyhow::Result<Self::Constant>;
+
+    /// Update the constants of the gas market. If the gas market is actor based, then it's recommended
+    /// to flush at EndBlock.
+    #[allow(dead_code)]
+    fn set_constants(&mut self, constants: Self::Constant);


We don't need these.

cryptoAtwill added 3 commits August 15, 2024 20:26

prepare and process

29cda12

unit tests

53ca9f2

end block

d3bbb35

cryptoAtwill requested a review from a team as a code owner August 22, 2024 12:36

cryptoAtwill commented Aug 22, 2024

View reviewed changes

raulk requested changes Aug 24, 2024

View reviewed changes

cryptoAtwill added 2 commits August 26, 2024 21:53

add validator tracking and gas premium distribution

65a6802

distribute validator reward

05ec86b

cryptoAtwill marked this pull request as draft August 27, 2024 09:45

cryptoAtwill added 5 commits August 27, 2024 17:48

rename methods

7fc7df0

use read only block store

ae6277b

add more tests

46f8e2b

fmt

f681494

more tests

4ff965b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): prepare + process + end block abci gas handling #1122

feat(node): prepare + process + end block abci gas handling #1122

cryptoAtwill commented Aug 22, 2024 •

edited

Loading

cryptoAtwill Aug 22, 2024

raulk Aug 24, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 26, 2024

raulk Aug 24, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 26, 2024

raulk Aug 24, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 26, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 26, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 27, 2024

raulk Aug 24, 2024

raulk Aug 24, 2024

cryptoAtwill Aug 27, 2024

raulk Aug 24, 2024

	self.call_fvm(msg, executor)?;
	self.apply_implicit_message(msg, executor)?;

feat(node): prepare + process + end block abci gas handling #1122

Are you sure you want to change the base?

feat(node): prepare + process + end block abci gas handling #1122

Conversation

cryptoAtwill commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoAtwill commented Aug 22, 2024 •

edited

Loading