Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

FM-316: Top down mint increase circ supply #330

Merged
merged 4 commits into from
Oct 26, 2023
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
16 changes: 14 additions & 2 deletions fendermint/app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use fendermint_vm_interpreter::chain::{
};
use fendermint_vm_interpreter::fvm::state::{
empty_state_tree, CheckStateRef, FvmExecState, FvmGenesisState, FvmQueryState, FvmStateParams,
FvmUpdatableParams,
};
use fendermint_vm_interpreter::fvm::store::ReadOnlyBlockstore;
use fendermint_vm_interpreter::fvm::{FvmApplyRet, FvmGenesisOutput};
Expand Down Expand Up @@ -725,9 +726,20 @@ where
let mut state = self.committed_state()?;
state.block_height = exec_state.block_height().try_into()?;
state.state_params.timestamp = exec_state.timestamp();
state.state_params.state_root = exec_state.commit().context("failed to commit FVM")?;

let state_root = state.state_root();
let (
state_root,
FvmUpdatableParams {
power_scale,
circ_supply,
},
_,
) = exec_state.commit().context("failed to commit FVM")?;

state.state_params.state_root = state_root;
state.state_params.power_scale = power_scale;
state.state_params.circ_supply = circ_supply;

let app_hash = state.app_hash();

tracing::debug!(
Expand Down
30 changes: 19 additions & 11 deletions fendermint/vm/interpreter/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
signed::{SignedMessageApplyRes, SignedMessageCheckRes, SyntheticMessage, VerifiableMessage},
CheckInterpreter, ExecInterpreter, GenesisInterpreter, ProposalInterpreter, QueryInterpreter,
};
use anyhow::{anyhow, Context};
use anyhow::{bail, Context};
use async_stm::atomically;
use async_trait::async_trait;
use fendermint_vm_actor_interface::ipc;
Expand Down Expand Up @@ -209,7 +209,8 @@ where
let (state, ret) = self
.inner
.deliver(state, VerifiableMessage::Synthetic(smsg))
.await?;
.await
.context("failed to deliver bottom up checkpoint")?;

// If successful, add the CID to the background resolution pool.
let is_success = match ret {
Expand All @@ -236,9 +237,7 @@ where
}
IpcMessage::TopDownExec(p) => {
if !provider.is_enabled() {
return Err(anyhow!(
"cannot execute IPC top-down message: parent provider disabled"
));
bail!("cannot execute IPC top-down message: parent provider disabled");
}

// commit parent finality first
Expand All @@ -249,26 +248,34 @@ where
finality.clone(),
&provider,
)
.await?;
.await
.context("failed to commit finality")?;

// error happens if we cannot get the validator set from ipc agent after retries
let validator_changes = provider
.validator_changes_from(prev_height + 1, finality.height)
.await?;
.await
.context("failed to fetch validator changes")?;

self.gateway_caller
.store_validator_changes(&mut state, validator_changes)?;
.store_validator_changes(&mut state, validator_changes)
.context("failed to store validator changes")?;

// error happens if we cannot get the cross messages from ipc agent after retries
let msgs = provider
.top_down_msgs_from(prev_height + 1, p.height as u64, &finality.block_hash)
.await?;
.await
.context("failed to fetch top down messages")?;

let ret = topdown::execute_topdown_msgs(&self.gateway_caller, &mut state, msgs)
.await?;
.await
.context("failed to execute top down messages")?;

atomically(|| {
provider.set_new_finality(finality.clone(), prev_finality.clone())
})
.await;

tracing::debug!("new finality updated: {:?}", finality);

Ok(((pool, provider, state), ChainMessageApplyRet::Ipc(ret)))
Expand Down Expand Up @@ -328,7 +335,8 @@ where
let (state, ret) = self
.inner
.check(state, VerifiableMessage::Synthetic(msg), is_recheck)
.await?;
.await
.context("failed to check bottom up resolve")?;

Ok((state, Ok(ret)))
}
Expand Down
55 changes: 48 additions & 7 deletions fendermint/vm/interpreter/src/fvm/state/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,22 @@ pub struct FvmStateParams {
pub power_scale: PowerScale,
}

/// Parts of the state which can be updated by message execution, apart from the actor state.
///
/// This is just a technical thing to help us not forget about saving something.
///
/// TODO: `base_fee` should surely be here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one, I like this approach! Yes, I was actually going to ask you why the base_fee wasn't included here.

I would include it for now even if it doesn't currently change as we may want to expose a callback to update it for users in the future. But maybe you have a reason not to include it yet, so happy either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't include it purely because there is no user story for how to update it. I created an issue, but haven't checked what's happening in Lotus or Forest. Maybe it's based on the last N blocks or something? Or maybe it's based on some voting, I don't know. I'd rather have a TODO stating it's not done, then code which suggests it might be done, which sends people looking all over to code just to confirm that nothing is happening.

But the history of it is already maintained, so as soon as we add it here, the compiler will tell us to update the value in app.rs and all will be well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep a static base_fee and not use what Lotus is doing, I would rather have a way for users to decide the base_fee function instead of giving them a really complicated one that they don't understand. So yes, let's leave it for the future. Thanks!

#[derive(Debug)]
pub struct FvmUpdatableParams {
/// The circulating supply changes if IPC is elabled and
aakoshh marked this conversation as resolved.
Show resolved Hide resolved
/// funds/releases are carried out with the parent.
pub circ_supply: TokenAmount,
/// Conversion between collateral and voting power.
/// Doesn't change at the moment but in theory it could,
/// and it doesn't have a place within the FVM.
pub power_scale: PowerScale,
}

pub type MachineBlockstore<DB> = <DefaultMachine<DB, FendermintExterns> as Machine>::Blockstore;

/// A state we create for the execution of all the messages in a block.
Expand All @@ -64,8 +80,11 @@ where
/// execution interpreter without having to add yet another piece to track at the app level.
block_hash: Option<BlockHash>,

/// Conversion between collateral and voting power.
power_scale: PowerScale,
/// State of parameters that are outside the control of the FVM but can change and need to be persisted.
params: FvmUpdatableParams,

/// Indicate whether the parameters have been updated.
params_dirty: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, I see this parameter updated but not read, is it used in other PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's a bit obscure. The only reason I added it was to use it in genesis where the commit, like the one here used to, returns just a CID but it's a unified one coming either from the StateTree or the FvmExecState. I didn't want to change that commit too, but I wanted to have a sanity check that there hasn't been some change that I am discarding, in which case this design would have to change too, e.g. by moving the fields from FvmGenesisOutput to the commit return value.

It's a bit convoluted, but I haven't found a nice way to do things. I tried with maintaining a copy of FvmStateParams, but not all those fields can change during a run, and for the ones that can be retrieved from the executor, there is always that ambiguity of which one to return. Now there is only one, because the circ_supply in the updatable part is write-only.

}

impl<DB> FvmExecState<DB>
Expand All @@ -90,7 +109,7 @@ where
// * base_fee; by default it's zero
let mut mc = nc.for_epoch(block_height, params.timestamp.0, params.state_root);
mc.set_base_fee(params.base_fee);
mc.set_circulating_supply(params.circ_supply);
mc.set_circulating_supply(params.circ_supply.clone());

// Creating a new machine every time is prohibitively slow.
// let ec = EngineConfig::from(&nc);
Expand All @@ -103,7 +122,11 @@ where
Ok(Self {
executor,
block_hash: None,
power_scale: params.power_scale,
params: FvmUpdatableParams {
circ_supply: params.circ_supply,
power_scale: params.power_scale,
},
params_dirty: false,
})
}

Expand Down Expand Up @@ -142,8 +165,9 @@ where
/// semantics we can hope to provide if the middlewares call each other: did it go
/// all the way down, or did it stop somewhere? Easier to have one commit of the state
/// as a whole.
pub fn commit(mut self) -> anyhow::Result<Cid> {
self.executor.flush()
pub fn commit(mut self) -> anyhow::Result<(Cid, FvmUpdatableParams, bool)> {
let cid = self.executor.flush()?;
Ok((cid, self.params, self.params_dirty))
}

/// The height of the currently executing block.
Expand All @@ -163,7 +187,7 @@ where

/// Conversion between collateral and voting power.
pub fn power_scale(&self) -> PowerScale {
self.power_scale
self.params.power_scale
}

/// Get a mutable reference to the underlying [StateTree].
Expand Down Expand Up @@ -201,6 +225,23 @@ where

Ok(emitters)
}

/// Update the circulating supply, effective from the next block.
pub fn update_circ_supply<F>(&mut self, f: F)
where
F: FnOnce(&mut TokenAmount),
{
self.update_params(|p| f(&mut p.circ_supply))
}

/// Update the parameters and mark them as dirty.
fn update_params<F>(&mut self, f: F)
where
F: FnOnce(&mut FvmUpdatableParams),
{
f(&mut self.params);
self.params_dirty = true;
}
}

impl<DB> HasChainID for FvmExecState<DB>
Expand Down
5 changes: 4 additions & 1 deletion fendermint/vm/interpreter/src/fvm/state/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ where
pub fn commit(self) -> anyhow::Result<Cid> {
match self.stage {
Stage::Tree(mut state_tree) => Ok(state_tree.flush()?),
Stage::Exec(exec_state) => exec_state.commit(),
Stage::Exec(exec_state) => match exec_state.commit()? {
(_, _, true) => bail!("FVM parameters are not expected to be updated in genesis"),
(cid, _, _) => Ok(cid),
},
}
}

Expand Down
2 changes: 2 additions & 0 deletions fendermint/vm/interpreter/src/fvm/state/ipc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,11 @@ impl<DB: Blockstore> GatewayCaller<DB> {
finality: IPCParentFinality,
) -> anyhow::Result<Option<IPCParentFinality>> {
let evm_finality = router::ParentFinality::try_from(finality)?;

let (has_committed, prev_finality) = self
.router
.call(state, |c| c.commit_parent_finality(evm_finality))?;

Ok(if !has_committed {
None
} else {
Expand Down
2 changes: 1 addition & 1 deletion fendermint/vm/interpreter/src/fvm/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ mod snapshot;
use std::sync::Arc;

pub use check::FvmCheckState;
pub use exec::{FvmExecState, FvmStateParams};
pub use exec::{FvmExecState, FvmStateParams, FvmUpdatableParams};
pub use genesis::{empty_state_tree, FvmGenesisState};
pub use query::FvmQueryState;
pub use snapshot::{Snapshot, V1Snapshot};
Expand Down
14 changes: 12 additions & 2 deletions fendermint/vm/interpreter/src/fvm/topdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::chain::TopDownFinalityProvider;
use crate::fvm::state::ipc::GatewayCaller;
use crate::fvm::state::FvmExecState;
use crate::fvm::FvmApplyRet;
use anyhow::Context;
use fendermint_vm_topdown::{BlockHeight, IPCParentFinality, ParentViewProvider};
use fvm_ipld_blockstore::Blockstore;
use fvm_shared::econ::TokenAmount;
Expand All @@ -28,14 +29,16 @@ where
} else {
(provider.genesis_epoch()?, None)
};

tracing::debug!(
"commit finality parsed: prev_height {prev_height}, prev_finality: {prev_finality:?}"
);

Ok((prev_height, prev_finality))
}

/// Execute the top down messages implicitly. Before the execution, mint to the gateway of the funds
/// transferred in the messages.
/// transferred in the messages, and increase the circulating supply with the incoming value.
pub async fn execute_topdown_msgs<DB>(
gateway_caller: &GatewayCaller<DB>,
state: &mut FvmExecState<DB>,
Expand All @@ -45,7 +48,14 @@ where
DB: Blockstore + Sync + Send + 'static,
{
let total_value: TokenAmount = messages.iter().map(|a| a.msg.value.clone()).sum();
gateway_caller.mint_to_gateway(state, total_value)?;

gateway_caller
.mint_to_gateway(state, total_value.clone())
.context("failed to mint to gateway")?;

state.update_circ_supply(|circ_supply| {
*circ_supply += total_value;
});

gateway_caller.apply_cross_messages(state, messages)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we already discussed this when we were reviewing top-down, and we said we would defer it to the future, but what is your take for when executing the message fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to re-read my comments:

  1. For crashing
  2. Against crashing

To reiterate the main points:

  • Individual messages in the batch should be allowed to fail without failing the whole batch
  • Failing the whole batch and moving on is unacceptable, especially if we can't roll back the validator changes and the parent height finality
  • But, the whole match might fail if it goes over the block gas limit
  • Therefore we should:
    • do all 3 parts as an atomic call to Solidity
    • let the whole thing fail as a transaction and be retried by another relayer
    • not execute immediately (and hit the block gas limit) but enqueue for execution and do it when we have room
    • then we can treat individual messages with their own gas limit

}
Loading