ava-labs / subnet-evm

Launch your own EVM as an Avalanche Subnet
https://docs.avax.network/subnets/create-a-fuji-subnet
GNU Lesser General Public License v3.0
244 stars 223 forks source link

More explicit support for `getVerifiedWarpMessage` as Anycast vs. Explicit Destination #868

Closed aaronbuchwald closed 1 year ago

aaronbuchwald commented 1 year ago

The Warp precompile currently supports getVerifiedWarpMessage, which will verify that a warp message was signed by a threshold of the source blockchainID's validator set. The address-to-address payload type specifies a destinationChainID as well, and the current implementation does not require that the destinationChainID matches the actual blockchainID of the blockchain receiving this message.

The motivation for this was to support Anycast messages in a lightweight way and defer verification of the destinationChainID to the caller.

This ticket is a feature request to support a more explicit "safe" version of getVerifiedWarpMessage that does this check for you and separates the Anycast and explicit destination handler functions.

Specifically, this proposes that we break it down into two functions:

michaelkaplan13 commented 1 year ago

This is an interesting idea, though my initial feeling that it is best to make as few assumptions as possible on how messages will be used by developers.

For instance, it's imaginable that a project would want to be able to receive warp messages sent to a specific chain that is neither their own chain ID or the anycast chain ID. It could be used for reporting messages sent or other uses cases aside from "anycasting". It'd be odd to have to use getAnyCastVerifiedWarpMessage in that case.

100% agree that it needs to be very clear that the destinationChainID for messages returned by getVerifiedWarpMessage can have any value and is not necessarily the current chain ID. Ultimately though, I think this may result in more code that needs to be maintained to offer the same functionality.

michaelkaplan13 commented 1 year ago

Trying to think how else an additional guard rail around the destinationChainID could be implemented here, which is what I think the real goal is.

One option could be to add an allowOtherChainIDs boolean parameter to getVerifiedWarpMessage. It would need to be explicitly set to true to allow chain IDs that are not the chain ID of the current chain. Protocol contracts such as TeleporterMessenger would set it to false rather than having to veryify the destinationChainID in their own logic.

This may make it more clear what the parameter is used for rather than having two different precompile methods.

richardpringle commented 1 year ago

I think (but don't know) that in most cases, if you (the smart-contract) already know the index of the message you're looking for, you should know the destinationChainID. The only way I see this breaking down is if a contract is looping over indices or something like that.

If we think that arbitrarily reading warp messages is a use-case we want to support (as we do now), I recommend the following interface:

    // only returns messages with `chainID` == this.getBlockchainID()
    function getVerifiedWarpMessage(uint32 index)
        external view
        returns (WarpMessage calldata message, bool valid);

    // only returns messages with `chainID` == `blockchainID` 
    function getVerifiedWarpMessage(uint32 index, bytes32 blockchainID)
        external view
        returns (WarpMessage calldata message, bool valid);

    // exact same functionality as the current `getVerifiedWarpMessage`    
    // return any message without validating the `chainID` 
    function getAnyVerifiedWarpMessage(uint32 index)
        external view
        returns (WarpMessage calldata message, bool valid);
michaelkaplan13 commented 1 year ago

I think (but don't know) that in most cases, if you (the smart-contract) already know the index of the message you're looking for, you should know the destinationChainID.

I agree that smart contracts should know the expected destinationChainID of any message, though I don't think it's necessarily related to the message index. In Teleporter for example, the message index is passed by the caller to TeleporterMessage.receiveCrossChainMessage as a parameter itself (because there isn't a good way for the contract to know which message is to be received), but it knows that for all messages the destinationChainID should be the current chain ID.

The interface you suggested seems totally reasonable to me. I think it's important to support possible use cases for receiving messages with arbitrary destinationChainIDs. I don't have a strong preference and would definitely defer to others to decide, but my slight preference is that I think it would be more clear for developers to still have a single interface method for receiving messages rather than having 3 with similar methods with slightly different functionality. IIUC, the same set of possible functionality is supported in either case, so ultimately does not make huge difference either way. 👍

aaronbuchwald commented 1 year ago

Discussing an alternative to this of removing the destinationChainID and destinationAddress field completely in favor of deferring that to Teleporter instead since the only thing Teleporter or any other protocol really needs from Warp is to encode the destinationChainID and destinationAddress since a protocol built on top can encode their own payload instead.

https://github.com/ava-labs/avalanchego/discussions/2050