Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 43 additions & 20 deletions fendermint/app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,40 @@ where
_ => Err(anyhow!("invalid app state json")),
}
}

fn create_check_state(&self) -> anyhow::Result<FvmExecState<ReadOnlyBlockstore<SS>>> {
let db = self.state_store_clone();
let state = self.committed_state()?;

// This would create a partial state, but some client scenarios need the full one.
// FvmCheckState::new(db, state.state_root(), state.chain_id())
// .context("error creating check state")?

let mut state = FvmExecState::new(
ReadOnlyBlockstore::new(db),
self.multi_engine.as_ref(),
state.block_height.try_into()?,
state.state_params,
)
.context("error creating check state")?;

// load txn priority calculator
let end = state.block_height() as u64;
let start = end
.saturating_sub(state.txn_priority_calculator().len() as u64)
.max(1);
for h in start..=end {
let Ok((state_params, _)) = self.state_params_at_height(FvmQueryHeight::Height(h))
else {
continue;
};
state
.txn_priority_calculator_mut()
.base_fee_updated(state_params.base_fee);
}

Ok(state)
}
}

// NOTE: The `Application` interface doesn't allow failures at the moment. The protobuf
Expand Down Expand Up @@ -561,22 +595,7 @@ where

let state = match guard.take() {
Some(state) => state,
None => {
let db = self.state_store_clone();
let state = self.committed_state()?;

// This would create a partial state, but some client scenarios need the full one.
// FvmCheckState::new(db, state.state_root(), state.chain_id())
// .context("error creating check state")?

FvmExecState::new(
ReadOnlyBlockstore::new(db),
self.multi_engine.as_ref(),
state.block_height.try_into()?,
state.state_params,
)
.context("error creating check state")?
}
None => self.create_check_state()?,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can avoid loading the 5 blocks state params for check_tx, to do this, we must change the commit to hold some historical check_state instead of purging everything. Not sure if we should do in this PR.

};

let (state, result) = self
Expand All @@ -589,9 +608,6 @@ where
.await
.context("error running check")?;

// Update the check state.
*guard = Some(state);

let mut mpool_received_trace = MpoolReceived::default();

let response = match result {
Expand All @@ -601,11 +617,18 @@ where
Ok(Err(InvalidSignature(d))) => invalid_check_tx(AppError::InvalidSignature, d),
Ok(Ok(ret)) => {
mpool_received_trace.message = Some(Message::from(&ret.message));
to_check_tx(ret)

let priority = state.txn_priority_calculator().priority(&ret.message);
let mut t = to_check_tx(ret);
t.priority = priority;
t
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: feed the priority as a param to to_check_tx?

}
},
};

// Update the check state.
*guard = Some(state);

mpool_received_trace.accept = response.code.is_ok();
if !mpool_received_trace.accept {
mpool_received_trace.reason = Some(format!("{:?} - {}", response.code, response.info));
Expand Down
14 changes: 14 additions & 0 deletions fendermint/vm/interpreter/src/fvm/state/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::collections::{HashMap, HashSet};

use crate::fvm::externs::FendermintExterns;
use crate::fvm::gas::BlockGasTracker;
use crate::fvm::state::priority::TxnPriorityCalculator;
use anyhow::Ok;
use cid::Cid;
use fendermint_actors_api::gas_market::Reading;
Expand Down Expand Up @@ -37,6 +38,8 @@ pub type ActorAddressMap = HashMap<ActorID, Address>;
/// The result of the message application bundled with any delegated addresses of event emitters.
pub type ExecResult = anyhow::Result<(ApplyRet, ActorAddressMap)>;

const DEFAULT_BASE_FEE_HISTORY: usize = 5;

/// Parts of the state which evolve during the lifetime of the chain.
#[serde_as]
#[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)]
Expand Down Expand Up @@ -114,6 +117,8 @@ where
params: FvmUpdatableParams,
/// Indicate whether the parameters have been updated.
params_dirty: bool,

txn_priority: TxnPriorityCalculator,
}

impl<DB> FvmExecState<DB>
Expand Down Expand Up @@ -163,6 +168,7 @@ where
power_scale: params.power_scale,
},
params_dirty: false,
txn_priority: TxnPriorityCalculator::new(DEFAULT_BASE_FEE_HISTORY),
})
}

Expand Down Expand Up @@ -267,6 +273,14 @@ where
self.params.power_scale
}

pub fn txn_priority_calculator(&self) -> &TxnPriorityCalculator {
&self.txn_priority
}

pub fn txn_priority_calculator_mut(&mut self) -> &mut TxnPriorityCalculator {
&mut self.txn_priority
}

pub fn app_version(&self) -> u64 {
self.params.app_version
}
Expand Down
1 change: 1 addition & 0 deletions fendermint/vm/interpreter/src/fvm/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub mod snapshot;
mod check;
mod exec;
mod genesis;
mod priority;
mod query;

use std::sync::Arc;
Expand Down
203 changes: 203 additions & 0 deletions fendermint/vm/interpreter/src/fvm/state/priority.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
// Copyright 2022-2024 Protocol Labs
// SPDX-License-Identifier: Apache-2.0, MIT

use crate::fvm::FvmMessage;
use fvm_shared::econ::TokenAmount;
use num_traits::{ToPrimitive, Zero};

/// The transaction priority calculator. The priority calculated is used to determine the ordering
/// in the mempool.
#[derive(Clone, Debug)]
pub struct TxnPriorityCalculator {
/// Ring buffer of base fee history
base_fee_history: Vec<Option<TokenAmount>>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need to keep a base fee history? In my head, this component would just know the current base fee and prioritise inclusion based on it -- past base fees are irrelevant. A transaction is either includable or not includable now. And we know for certain that after a block is committed, CometBFT will recheck all pending transactions before proposing the next block, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this was quite early discussion on one of the huddles to use the lowest base fee in recent history to let slightly more transaction to go through to avoid empty blocks. An array is also tuneable in how base fee can be selected. The requirement is updated slightly and base fee selection is now includable/non-includable. Then the most recent base fee should be enough. I can update this accordingly.

Copy link
Copy Markdown
Contributor

@raulk raulk Dec 14, 2024

Choose a reason for hiding this comment

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

We'll need to fix this. The base fee determines inclusion and the burn rate (network fee). By no means should a transaction be included if its gas fee cap is below the current base fee for the block it's included in. That's a consensus fault.

/// Next slot in the ring buffer
next_slot: usize,
}

impl TxnPriorityCalculator {
pub fn new(size: usize) -> Self {
let mut v = Vec::with_capacity(size);
for _ in 0..size {
v.push(None);
}
Self {
base_fee_history: v,
next_slot: 0,
}
}

pub fn len(&self) -> usize {
self.base_fee_history.len()
}

pub fn base_fee_updated(&mut self, base_fee: TokenAmount) {
self.base_fee_history[self.next_slot] = Some(base_fee);
self.next_slot = (self.next_slot + 1) % self.base_fee_history.len();
}

pub fn priority(&self, msg: &FvmMessage) -> i64 {
let base_fee = self.lowest_base_fee();

if msg.gas_fee_cap < base_fee {
return i64::MIN;
}

let effective_premium = msg.gas_premium.clone().min(&msg.gas_fee_cap - base_fee);
effective_premium.atto().to_i64().unwrap_or(i64::MAX)
}

fn lowest_base_fee(&self) -> TokenAmount {
let mut out: Option<TokenAmount> = None;
for v in &self.base_fee_history {
let Some(v) = v.as_ref() else { continue };

match out {
Some(min) => out = Some(min.min(v.clone())),
None => out = Some(v.clone()),
}
}

out.unwrap_or(TokenAmount::zero())
}
}

#[cfg(test)]
mod tests {
use crate::fvm::state::priority::TxnPriorityCalculator;
use crate::fvm::FvmMessage;
use fvm_shared::address::Address;
use fvm_shared::bigint::BigInt;
use fvm_shared::econ::TokenAmount;

fn create_msg(fee_cap: TokenAmount, premium: TokenAmount) -> FvmMessage {
FvmMessage {
version: 0,
from: Address::new_id(10),
to: Address::new_id(12),
sequence: 0,
value: Default::default(),
method_num: 0,
params: Default::default(),
gas_limit: 0,
gas_fee_cap: fee_cap,
gas_premium: premium,
}
}

#[test]
fn base_fee_update_works() {
let mut cal = TxnPriorityCalculator::new(3);

assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(0));

// [10, None, None]
cal.base_fee_updated(TokenAmount::from_atto(10));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(10));
assert_eq!(
cal.base_fee_history,
vec![Some(TokenAmount::from_atto(10)), None, None]
);

// [10, 20, None]
cal.base_fee_updated(TokenAmount::from_atto(20));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(10));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(10)),
Some(TokenAmount::from_atto(20)),
None
]
);

// [10, 20, 5]
cal.base_fee_updated(TokenAmount::from_atto(5));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(5));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(10)),
Some(TokenAmount::from_atto(20)),
Some(TokenAmount::from_atto(5)),
]
);

// [6, 20, 5]
cal.base_fee_updated(TokenAmount::from_atto(6));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(5));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(6)),
Some(TokenAmount::from_atto(20)),
Some(TokenAmount::from_atto(5)),
]
);

// [6, 100, 5]
cal.base_fee_updated(TokenAmount::from_atto(100));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(5));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(6)),
Some(TokenAmount::from_atto(100)),
Some(TokenAmount::from_atto(5)),
]
);

// [6, 100, 10]
cal.base_fee_updated(TokenAmount::from_atto(10));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(6));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(6)),
Some(TokenAmount::from_atto(100)),
Some(TokenAmount::from_atto(10)),
]
);

// [10, 100, 10]
cal.base_fee_updated(TokenAmount::from_atto(10));
assert_eq!(cal.lowest_base_fee(), TokenAmount::from_atto(10));
assert_eq!(
cal.base_fee_history,
vec![
Some(TokenAmount::from_atto(10)),
Some(TokenAmount::from_atto(100)),
Some(TokenAmount::from_atto(10)),
]
);
}

#[test]
fn priority_calculation() {
let mut cal = TxnPriorityCalculator::new(3);

cal.base_fee_updated(TokenAmount::from_atto(10));
cal.base_fee_updated(TokenAmount::from_atto(20));
cal.base_fee_updated(TokenAmount::from_atto(30));

// lowest base fee is 10

let msg = create_msg(TokenAmount::from_atto(1), TokenAmount::from_atto(20));
assert_eq!(cal.priority(&msg), i64::MIN);

let msg = create_msg(TokenAmount::from_atto(10), TokenAmount::from_atto(20));
assert_eq!(cal.priority(&msg), 0);

let msg = create_msg(TokenAmount::from_atto(15), TokenAmount::from_atto(20));
assert_eq!(cal.priority(&msg), 5);

let msg = create_msg(TokenAmount::from_atto(40), TokenAmount::from_atto(20));
assert_eq!(cal.priority(&msg), 20);

let msg = create_msg(
TokenAmount::from_atto(BigInt::from(i128::MAX)),
TokenAmount::from_atto(BigInt::from(i128::MAX)),
);
assert_eq!(cal.priority(&msg), i64::MAX);
}
}