Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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
1 change: 1 addition & 0 deletions fendermint/vm/interpreter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ tokio-util = { workspace = true }
arbitrary = { workspace = true, optional = true }
quickcheck = { workspace = true, optional = true }
rand = { workspace = true, optional = true }
lazy_static = { workspace = true }

[dev-dependencies]
quickcheck = { workspace = true }
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
211 changes: 211 additions & 0 deletions fendermint/vm/interpreter/src/fvm/state/priority.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
// Copyright 2022-2024 Protocol Labs
// SPDX-License-Identifier: Apache-2.0, MIT

use crate::fvm::FvmMessage;
use fvm_shared::bigint::BigInt;
use fvm_shared::econ::TokenAmount;
use lazy_static::lazy_static;
use num_traits::{ToPrimitive, Zero};
use std::str::FromStr;

lazy_static! {
// Max U256
static ref MAX_GAS: TokenAmount = TokenAmount::from_atto(BigInt::from_str("115792089237316195423570985008687907853269984665640564039457584007913129639935").unwrap());
}

/// 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 {
let i = msg.gas_fee_cap.clone() - base_fee + msg.gas_premium.clone();
i.atto()
.min(&BigInt::from(i64::MAX))
.to_i64()
.expect("clipped to i64 max")
} else {
0
}
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.

  1. These kinds of conditionals are easier to follow when written as an early return. If the gas_fee_cap is below the base fee, return 0.

  2. Are we sure 0 is the right value here? Why not return i64::MIN if the message is non-includable? Right now messages with gas_fee_cap == base_fee && gas_premium == 0 would rank exactly the same as non-includable messages. Seems wrong?

  3. Adding the gas premium to the delta between the gas_fee_cap and the base fee is incorrect. The gas_fee_cap includes the payable base fee and the gas premium. When the message is includable, our priority should be proportional to the effective gas premium. See how the effective gas premium is calculated in ref-fvm; copy the logic here.

  4. i64::MAX atto is equal to 9.223372036854775807 base units. We are probably safe in clipping the effective premium to i64::MAX, since nobody sane will set such a high premium, but I'm uncomfortable with the panic here. I suggest this instead, which saturates to i64::MAX:

    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), 0);

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

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

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