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

FM-13: Message interpreter #14

Merged
merged 16 commits into from
Feb 6, 2023
Merged

FM-13: Message interpreter #14

merged 16 commits into from
Feb 6, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Feb 3, 2023

Closes consensus-shipyard/ipc#289

Adds a skeleton of the message handling logic. There is

  • an Interpreter with begin/deliver/end methods
  • interpreter for the FVM Message type
  • interpreter for SignedMessage: checking the signature which should be okay unless the validator is messing with us
  • interpreter for ChainMessage: eventually will resolve CIDs to SignedMessage or CrossMessage; here I plan to use STM to consult the rest of the application state.
  • interpreter for raw bytes
  • FvmState which contains an executor for the duration of the block processing cycle
  • App to interface between the above and ABCI, and maintain the execution state.

In the future I'll have a similar interpreter for: check_tx: it will have a similar check_state in the App, but it will be simplified, probably just balances and nonces, with its value reset every time there is a commit.

In Milestone 2, a similar interpreter will be made to handle proposals, which is another place where async operations can be relevant:

  • when looking up the availability of CIDs and
  • when looking up the state of the finality of top-down message in the parent subnet

There are unfinished bits, to be followed up in new PRs, because this one is already too big:

  • Store the application state in RocksDB; shouldn't this be in a separate column family from the general IPLD store?
  • Implement cron on begin; this needs something like the actor_interfaces project in Forest, to know which method number to call and what the actor ID of the cron actor is.
  • Map events to the output in begin and deliver; I'll do this once the fields of StampedEvent are made public.
  • Figure out how to maintain the circ_supply field.

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

This is a rubber stamp at this point, I am more a viewer than a proper reviewer (let me know if in any PR you want me to spend more cycles), but I really like where this is heading. Thanks!

state.base_fee,
state.circ_supply,
)
.expect("error creating new state");
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you are using expect instead of propagating an error (I guess this has to do with the signature of the methods in the Application trait, but would this kill the whole application?

Copy link
Contributor Author

@aakoshh aakoshh Feb 3, 2023

Choose a reason for hiding this comment

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

Yes, that's right, the Application trait won't let me do otherwise. Today I thought I'd change it; it's only like this because the tendermint-rs project has a similar trait (just synchronous). It was created in https://github.com/consensus-shipyard/fendermint/issues/3

However, the way the tower-abci library I'm using to host the service works is that it would panic if I returned the Response::Exception kind that Tendermint allows. Even if I did, the result would be Tendermint dropping the connection here, and then we'd have an application nobody talks to any more.

So yeah, panicking here will kill the application, which will show up in the Tendermint logs as well because the connections will be lost.

/// example on the outermost layer the input message can be a mix
/// of self-contained messages and CIDs proposed for resolution
/// or execution, while in the innermost layer it's all self-contained.
/// Some interpreters would act like middlewares to resolve CIDs into
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this, this is going to be key, we should consider using this interpreter abstraction also as part of the IPC agent to get this translation.

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 went back and forth a lot on this. It's not clear where to put state exactly: into the middleware, or into the State that goes in and out. I wanted to change it to return effects explicitly, and otherwise be synchronous, but then I realised that I will have to resolve CIDs on the fly anyway, when they are given to me for execution. So, for now, I'll see where it goes.

///
/// This is where we can apply end-of-epoch processing, for example to process staking
/// requests once every 1000 blocks.
async fn end(&self, state: Self::State) -> anyhow::Result<(Self::State, Self::EndOutput)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This determines the specific callback to trigger at the end of a block for a specific message, right?

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'm not sure what you mean by "for a specific message" 🤔

This happens at the end of the block, when all messages have been processed. The concrete use case is that we look at the height of the block (part of state) and if, say, height % 1000 == 0 then we produce a checkpoint. Because it's async, it can even send it if we want, but it can definitely add it to the ledger as well. Another thing that will happen here is that all the staking changes which were buffered on the ledger during the last 1000 blocks are applied, to calculate a new power distribution and return the validator set changes to Tendermint.

So, there is no specific message at this point, but rather what the ledger state dictates.

@aakoshh aakoshh merged commit e3ef4cc into master Feb 6, 2023
@aakoshh aakoshh deleted the fm-13-message-interpreter branch February 6, 2023 11:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message interpreter
2 participants