WIP: Implement Mailbox Contract for Sealevel-Based Chains #1345

sloboste · 2022-11-30T00:37:00Z

Description

What's included in this PR?

Bare bones implementation of mailbox contracts for Sealevel VM based chains (Solana).

Still some remaining FIXMEs, but wanted to put this up as a draft prior to our call tomorrow for visibility.

Drive-by changes

Are there any minor or drive-by changes also included?

Copy pasted and extended part of abacus-core to get around dependency conflicts with ethers-rs.

Related issues

#1283

Depends on this PR:
paritytech/parity-common#698

Backward compatibility

Are these changes backward compatible?

Yes. Additive only.

Are there any infrastructure implications, e.g. changes that would prohibit deploying older commits using this infra tooling?

None

Testing

Manual with test client
Unit tests for mailbox contract

tkporter

I think a lot of these were answered / talked about in our call just now, but sharing before I sign off today

tkporter · 2022-11-30T16:04:27Z

rust/sealevel/programs/mailbox/src/processor.rs

+
+    let id = message.id();
+    outbox.tree.ingest(id);
+    msg!("Hyperlane outbox: {}", &formatted);


Is it easy to index these from old blocks?

Just thinking of how Wormhole did it and wondering if you felt like the writing to an account was overkill and that logs are sufficient

web socket clients can subscribe to get updates so easy to keep pace but allowing a new node to catch up would be more difficult currently afaik. Would likely need to store off chain for longer periods of time, write to an account (will need to do that for large messages at some point, though probably would not make sense to keep the message around for that long), or similar

Imo we should just write this to an account like this guy solana-labs/solana#23653 (comment) also went with
Opened this to track it #1590

makes sense - also encountered the noop cpi recently. Will look into that

tkporter · 2022-11-30T16:17:03Z

rust/sealevel/programs/mailbox/src/processor.rs

+    let mut serialized = vec![];
+    message.write_to(&mut serialized)?;
+
+    // The hyperlane validator's indexer subscribes to transaction logs for this account and


nit: not really important but fwiw the validators just need to poll the latest checkpoint and sign it if it's change, and the relayers are the one that actually need to index messages

tkporter · 2022-11-30T16:23:36Z

rust/sealevel/programs/mailbox/src/processor.rs

+        nonce: count,
+        origin_domain: config.local_domain,
+        sender: H256(dispatch.sender.to_bytes()),
+        destination_domain: dispatch.destination_domain.into(),


nit: might be wrong but I think the into isn't needed as they're both u32s

will run clippy and fix

tkporter · 2022-11-30T16:26:12Z

rust/sealevel/programs/mailbox/src/processor.rs

+    let (formatted, message_len) = committed_message
+        .format()
+        .map_err(|_| ProgramError::from(Error::MalformattedCommittedMessage))?;
+    if message_len > MAX_MESSAGE_BODY_BYTES {


nit: we've been enforcing this on the message body and not the full encoded message size (see https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/solidity/contracts/Mailbox.sol#L110)

good catch, will fix

rust/sealevel/programs/mailbox/src/processor.rs

tkporter · 2022-11-30T16:45:15Z

rust/sealevel/programs/mailbox/src/processor.rs

+
+    // The hyperlane validator's indexer subscribes to transaction logs for this account and
+    // records the serialized messages.
+    let committed_message = RawCommittedMessage {


heads up that I think this type's now removed for v2

(Generally now that main is v2, probably worth pulling from main)

will rebase fix that

tkporter · 2022-11-30T17:08:11Z

rust/sealevel/programs/mailbox/src/processor.rs

+    }
+    let config = ConfigAccount::fetch(&mut &config_account.data.borrow_mut()[..])?.into_inner();
+
+    let message = AbacusMessageV2::read_from(&mut std::io::Cursor::new(process.message))


nit: I think you can get away without needing the Cursor by using a slice? May be wrong tho

will double check on that

tkporter · 2022-11-30T17:11:09Z

rust/sealevel/programs/mailbox/src/error.rs

+    UnsupportedDomain = 4,
+    #[error("Incorrect destination domain")]
+    IncorrectDestinationDomain = 5,
+    #[error("Message has already been dispatched/processed")]


Dispatched messages should always be unique because of the nonce, so this should only apply for processing. Maybe worth a rename to something closer to MessageAlreadyProcessed or something

makes sense, will rename

tkporter · 2022-11-30T17:20:41Z

rust/sealevel/programs/mailbox/src/instruction.rs

+#[derive(BorshDeserialize, BorshSerialize, Debug, PartialEq)]
+pub struct InboxProcess  {
+    pub root: H256,
+    pub index: [u8; 32], // FIXME what is this again? Can this be a u32?


Yeah this can be a u32 - this is supposed to be the "checkpoint index" which is equal to the tree's count - 1. You can think of the root and the index logically as 1 entity

But now that we have ISMs in v2, this whole struct could be reduced to:

pub struct InboxProcess { // Arbitrary bytes that are passed into the ISM - // encoded in here would be the root, index, signatures, the proof, etc // but it all depends on the ISM. The relayer will know how to // format this data correctly. See here for the format in Solidity // that should apply similarly here // https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/solidity/contracts/libs/MultisigIsmMetadata.sol#L4 pub metadata: Vec<u8>, pub message: Vec<u8>, // Encoded message }

Ofc this is also in a world where we don't need to worry about hitting the max message size, so maybe they need to be split up or something into 2 separate txs, one to verify with the ISM and one to actually process the message? Guess we'll chat soon

okay perfect, will change that datatype and move to the more generic process instruction data

rust/sealevel/programs/mailbox/src/instruction.rs

tkporter · 2022-11-30T18:16:11Z

Also great job - super impressed with the progress

tkporter

Mostly questions or small things

No need to have strict answers for many of the FIXMEs but before merging you should grab some of the new v2 code from main for a bunch of the copied over abacus-core stuff

Also a lot of these things we've talked a little bit about before, so apologies for duplicating conversations. A lot of this is just to be able to refer to later & for my own learning

tkporter · 2022-12-02T12:40:46Z

rust/sealevel/programs/mailbox/src/processor.rs

+) -> Result<(), ProgramError> {
+    let accounts_iter = &mut accounts.iter();
+
+    // FIXME need some way to tie the config account to the inbox account so that we don't allow


First things that come to mind for me for this - would love your thoughts to help me understand where I'm probably very wrong:

Create the config account as a PDA when initializing this program - iiuc the pubkey of PDAs are deterministic, so as long as they're deterministically generated based off the current program_id and some seed like "config", we should be able to verify the authenticity of the config account?
I see that the config / inbox / outbox are currently PDAs but their base pubkey is the payer - I'm wondering if the base pubkey should be the mailbox's program id (if that's possible)

The config account could be created before deploying the Mailbox, and then the config account pubkey could be hardcoded in the code of the mailbox. Is this possible / a common practice? Feels janky though

tkporter · 2022-12-02T14:03:58Z

rust/sealevel/programs/mailbox/src/processor.rs

+    if inbox_account.owner != program_id {
+        return Err(ProgramError::IncorrectProgramId);
+    }
+    let mut inbox = InboxAccount::fetch(&mut &inbox_account.data.borrow_mut()[..])?.into_inner();


Thinking about how we'll need:

Protection against someone supplying a malicious inbox account that allows them to pretend like a message hasn't already been processed

How access control on the recipient's handle function will work

Can this be done by making the inbox account a PDA? Where we use invoke_signed when calling a recipient's handle, and they have access control on their side to ensure that their handle function can only be called when the Inbox PDA has been signed? With the similar idea of having the inbox account's PDA's pubkey be deterministic and based off the Mailbox's program_id

(I may be way off here)

Yes, I think using a PDA here is the the way to ensure both that the inbox account's data was actually written by the inbox contract and to also endure that the ism and recipient can only be called by the inbox contract. Not sure if the jargon was used 100% correctly there but we're on the same page about what needs to happen afaict

tkporter · 2022-12-02T14:42:51Z

rust/sealevel/programs/mailbox/src/processor.rs

+    program_id: &Pubkey,
+    accounts: &[AccountInfo],
+    process: InboxProcess,
+) -> Result<[u8; 32], ProgramError> {


I don't think we need to return the message ID when processing (though if you were thinking about a particular use case I'm open to keeping it)

will remove

tkporter · 2022-12-02T14:59:47Z

rust/sealevel/programs/mailbox/src/lib.rs

+pub use abacus_core;
+
+// FIXME Read these in at compile time?
+solana_program::declare_id!("8TibDpWMQfTjG6JxvF85pxJXxwqXZUCuUx3Q1vwojvRh");


How does this work? I assume this is what the deployed program's pubkey is? Is this something that can be chosen before deploying?

yeah, it's the program id

Yeah, ideally I'd like to put in the machinery to define it in a config file - experimented with that real quick the other day but was unsuccessful. might need to extend the macro but will have to take another look

tkporter · 2022-12-02T15:00:24Z

rust/sealevel/programs/mailbox/src/lib.rs

+// FIXME Read these in at compile time?
+solana_program::declare_id!("8TibDpWMQfTjG6JxvF85pxJXxwqXZUCuUx3Q1vwojvRh");
+
+// FIXME set a sane default - seems like this is intended to be dynamic?


correct - I think this FIXME is resolved?

i'd keep the TODO to define a sane default - the current default is a dummy ism

tkporter · 2022-12-02T15:37:30Z

rust/sealevel/programs/mailbox/src/accounts.rs

+pub const CONFIG_ACCOUNT_SIZE: usize = 1024;
+
+// FIXME 10MB account size limits the number of messages we can store to ~30k. Figure out how to
+// extend this. This might get expensive...


A couple questions:

would we need to pay up from for 10MB of rent-exempt storage on this account? or can you expand account sizes

would the alternative be a PDA for each message where the seed is the message ID and the data in the PDA specifies if it's been delivered?

whoops this should have said ~300k entries fit in the hash set while staying under 10MB.

Accounts can be resized up to the 10MB limit. So if we need to permanently keep track of all the message hashes, we will eventually need to consider what happens when we fill up an entire account.

I think (2) might work.

Also thinking about storing both a hash set and the address of the previously filled up account in the inbox account. The awkward thing there would be how to inform relayers of the currently used inbox account and how to avoid having an ever expanding list of accounts that could be used by the inbox process transaction... So probably not a good solution.

I'd be curious if it'd make sense to put some sort of expiration date on messages that are to be delivered as to not use an unbounded amount of storage

rust/sealevel/programs/mailbox/src/accounts.rs

tkporter · 2022-12-02T15:46:17Z

rust/sealevel/programs/mailbox/src/accounts.rs

+
+/// Account data structure wrapper type that handles initialization and (de)serialization.
+///
+/// (De)serialization is done with borsh and the "on-disk" format is as follows:


Is it sufficient to assume that the presence of some bytes (i.e. the account's data not being empty) implies initialization?

yeah that's probably sufficient however it would likely be wise to borrow from the anchor framework here and introduce a magic byte sequence that indicates the data type to help guard against deserializing garbage data: https://github.com/coral-xyz/anchor/blob/master/lang/src/lib.rs

well actually no, probably not in some cases

Consider the case where we store an 32 bit integer in the account. How would we discern value = 0 from account has been created but no value has been set yet?

you'd have to require that all data stored in the account has a valid default value, on creation

tkporter · 2022-12-02T15:50:05Z

rust/sealevel/programs/mailbox/src/processor.rs

+        sovereign_data: ism_data, // FIXME is this all we need to pass in? Need proof too?
+        message: message.message_body.clone(),
+    };
+    if config.ism != *next_account_info(accounts_iter)?.key {


Eventually we'll want to get the ISM specified by the recipient and fall back to the default ISM if the recipient returns some kind of empty value (like a "zero" account)

But ofc we can do that in a follow up, just wanted to flag to not forget

tkporter · 2022-12-02T16:51:45Z

rust/sealevel/abacus-core/src/types/message.rs

+use crate::types::H256;
+use sha3::{Digest, Keccak256};
+use sha3::digest::Update;
+


Recommend taking a look at the new types in main (which includes v2 now) and copying those over, as I think some are stale

tkporter

This is awesome! This PDA stuff is super cool, great job!

I reviewed all of processor.rs but will spend some time reviewing everything else as well, just wanted to share my comments so far.

Atm I'm thinking it may make sense to make a feature branch or something with the sealevel contracts, which we can turn this branch into. I can break down what I think the minimal next steps are to have a working version (e.g. with #1363, #1371, and #1590 in mind), I can share this I think on Monday. Then maybe we can chat synchronously about the path forward?

rust/sealevel/programs/mailbox/src/instruction.rs

rust/sealevel/programs/mailbox/src/processor.rs

tkporter · 2023-01-13T12:55:15Z

rust/sealevel/programs/mailbox/src/processor.rs

+            program_id,
+        ),
+        &[
+            system_program.clone(),


Probably dumb questions, but I was looking at this example here https://docs.rs/solana-program/latest/solana_program/program/fn.invoke_signed.html#examples

I see some asserts, e.g:

assert!(payer.is_writable); assert!(payer.is_signer); assert!(vault_pda.is_writable); assert_eq!(vault_pda.owner, &system_program::ID);

Are those payer ones required? Or is it implied in this situation that that's the case?
And then for the pda stuff - are these required? I see assert_eq!(vault_pda.owner, &system_program::ID); as possibly a way to check if the pda has been created already, or is that also implied by the create_account instruction?

In that example they don't pass in the system_program as an account here, any idea why?

The conditions in the payer asserts are implied and the program should fail in other places if they are not true but it probably wouldn't hurt to add those checks.

With regards to checking the PDA owner prior to creating it, I'm assuming that is done to ensure that the account doesn't already exist and is not owned by some malicious account. I will have to dig into that a bit more however because I'm not sure about the behavior around yet to be created accounts.

Actually not sure on why create_account() doesn't take the system_program in that example - the example code I was referring to does. I will follow up on this one as well.

Passing in system_program to create_account is unnecessary as long as the calling instruction has been passed system_program. The system program doesn't need itself passed in as an account

tkporter · 2023-01-13T15:35:23Z

rust/sealevel/programs/mailbox/src/processor.rs

+    ism: InboxSetDefaultModule,
+) -> ProgramResult {
+    let accounts_iter = &mut accounts.iter();
+


Similar question on the possibility of consolidating some of e.g. here through 364 into a get_inbox fn or something to share between inbox_.* functions

tkporter · 2023-01-13T15:45:17Z

rust/sealevel/programs/mailbox/src/processor.rs

+    for pubkey in &inbox.ism_accounts {
+        let meta = AccountMeta {
+            pubkey: *pubkey,
+            // TODO this should probably be provided up front...


wdym by this?

oh is this saying that sometimes the accounts may actually be a signer or writable and we should support that?

yes, we may want to allow an ism to write to some accounts

tkporter · 2023-01-13T15:56:43Z

rust/sealevel/programs/mailbox/src/processor.rs

+    invoke_signed(&recieve, &recp_accounts, &[auth_seeds])?;
+    msg!("Hyperlane inbox: {:?}", id);
+
+    InboxAccount::from(inbox).store(inbox_account, true)?;


should this be before the invoke_signed calls? Storing the update to the delivered map after the invokes feels like it's susceptible to reentrancy (though maybe this isn't of concern in the Solana world as it is in the EVM?)

I would argue the opposite in fact. If we recurse prior to storing, then we will burn through the whole compute budget rather than error out with something like message already delivered.

tkporter · 2023-01-13T16:57:20Z

rust/sealevel/programs/mailbox/src/processor.rs

+// TODO add more strict checks on permissions for all accounts that are passed in, e.g., bail if
+// we expected an account to be read only but it is writable. Could build this into the AccountData
+// struct impl and provide more fetch methods.
+fn inbox_process(


On the note of reentrancy - are reentrancy guards used in Solana? We have a reentrancy guard on our process function in the EVM

ah so the recipient cannot be the mailbox contract? We could add one however there is a finite compute budget and if the payer sends a txn that will cause the contract to recurse infinitely, it will burn through the compute budget. It may make more sense to leave as is to disincentivize making recursive calls like that.

rust/sealevel/programs/mailbox/src/accounts.rs

sloboste · 2023-01-13T19:35:52Z

This is awesome! This PDA stuff is super cool, great job!

I reviewed all of processor.rs but will spend some time reviewing everything else as well, just wanted to share my comments so far.

Atm I'm thinking it may make sense to make a feature branch or something with the sealevel contracts, which we can turn this branch into. I can break down what I think the minimal next steps are to have a working version (e.g. with #1363, #1371, and #1590 in mind), I can share this I think on Monday. Then maybe we can chat synchronously about the path forward?

Feature branch sounds like the right approach to me. I'll rebase and PR to that branch when you push it

This reverts commit edca678.

tkporter reviewed Nov 30, 2022

View reviewed changes

tkporter reviewed Dec 2, 2022

View reviewed changes

sloboste force-pushed the steven/sealevel-mailbox branch from e9336ad to 7f7a5aa Compare December 14, 2022 00:31

sloboste force-pushed the steven/sealevel-mailbox branch 6 times, most recently from b15b152 to c8290a9 Compare January 12, 2023 17:37

tkporter reviewed Jan 13, 2023

View reviewed changes

Steven Sloboda added 4 commits January 17, 2023 14:07

Implement Hyperlane Sealevel mailbox contract.

e8e6a17

Refactor mailbox contracts to use PDAs where appropriate.

4390b52

FIXME drop this commit - use local solana repo.

edca678

Revert "FIXME drop this commit - use local solana repo."

a3649ff

This reverts commit edca678.

sloboste force-pushed the steven/sealevel-mailbox branch from c8290a9 to a3649ff Compare January 17, 2023 20:12

tkporter mentioned this pull request Mar 21, 2023

Pull core rust types into their own crate with WASM-compatible dependencies #1977

Closed

WIP: Implement Mailbox Contract for Sealevel-Based Chains #1345

Are you sure you want to change the base?

WIP: Implement Mailbox Contract for Sealevel-Based Chains #1345

Conversation

sloboste commented Nov 30, 2022

Description

Drive-by changes

Related issues

Backward compatibility

Testing

tkporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkporter commented Nov 30, 2022

tkporter left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloboste Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloboste commented Jan 13, 2023

tkporter left a comment •

edited

Loading

sloboste Dec 7, 2022 •

edited

Loading