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

FM-190: Check relayed bottom-up checkpoints #198

Merged
merged 3 commits into from
Aug 23, 2023
Merged

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Aug 16, 2023

Closes consensus-shipyard/ipc#314
Depends on #187

Introduces a VerifiableMessage and SynteticMessage which are used to expand the messages handled by the SignedMessageInterpreter to include one that is artificially constructed by the interpreter to make use of existing services.

In particular, the signed relayed bottom up checkpoint is turned into an FVM message, with the relayer signature assumed to be over the CID of the relay-message itself. The signature of this synthetic message is verified by the SignedMessageInterpreter before it passes it into the inner FvmMessageInterpreter which checks the existence of the relayer account and the nonce.

The PR adds missing gas fields to the relayed message.

@aakoshh aakoshh changed the base branch from main to fm-173-bottom-up-ckpt-msg August 16, 2023 15:40
@aakoshh aakoshh marked this pull request as ready for review August 16, 2023 15:43
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.

A few minor questions, but LGTM.

/// The CID of the original message (assuming here that that's what was signed).
orig_cid: cid::Cid,
/// The signature over the original CID.
signature: Signature,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double-check, this is the original signature of the message, right? Should we maybe call it WrappedMessage instead of SyntheticMessage? (weak opinion weakly held, but synthetic seemed like "artificial" to me :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an artificial message, that's exactly what I was trying to convey, that it's just a transient technical message to piggy-back on the signature checks and FVM message execution. The construction of the FVM message from the original is synthesising an artificial message that nobody sent.

It's indeed wrapping something but that wouldn't say why; so does the other one after all.

You are correct that signature is the signature over the original message, in particular the CID of the original message.

We could get rid of this if we used FVM messages, in which case instead of synthesising a message in the ChainMessageInterpreter from an IPC specific one, we would parse an FVM message into an IPC message to look for CIDs to resolve.

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 this is fine for now, and now I understand the rationale behind the name. Thanks!


pub fn verify(&self, chain_id: &ChainID) -> Result<(), SignedMessageError> {
let mut data = self.orig_cid.to_bytes();
data.extend(chain_id_bytes(chain_id).iter());
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the chainID was a prefix instead of a suffix of the message cid. Out of a curiosity, is this Fendermint-specific or Filecoin also does this?

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 have no idea what Filecoin does, I did this in #115

Ethereum seems to play with the signature itself, which seemed so complicated I didn't think we need to replicate it in Fendermint. The ethers library takes care of it for us, but for regular messages I do the suffix. Is prefix safer?

fendermint/vm/message/src/ipc.rs Show resolved Hide resolved
@aakoshh aakoshh mentioned this pull request Aug 22, 2023
4 tasks
Base automatically changed from fm-173-bottom-up-ckpt-msg to main August 23, 2023 13:30
@aakoshh aakoshh merged commit f7de439 into main Aug 23, 2023
1 check passed
@aakoshh aakoshh deleted the fm-190-check-bottom-up branch August 23, 2023 13:41
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.

Implement CheckInterpreter for Ipc::BottomUpResolve
2 participants