Block hash publisher #54
Conversation
cam-schultz
requested review from
michaelkaplan13,
minghinmatthewlam,
gwen917 and
geoff-vball
as code owners
| @@ -232,6 +237,8 @@ func (s *SourceSubnet) Validate() error { | |||
| return fmt.Errorf("invalid message contract address in EVM source subnet: %s", messageContractAddress) | |||
| } | |||
| } | |||
| case EVM_BLOCKHASH: | |||
There was a problem hiding this comment.
It seems odd that Blockhash is considered its own VM type. Are we overloading the term "VM" here?
| ) | ||
|
|
||
| const ( | ||
| publishBlockHashGasLimit = 275000 |
There was a problem hiding this comment.
Is this temporary, or do we know if this will be enough?
| return nil, err | ||
| } | ||
| var messageConfig Config | ||
| if err := json.Unmarshal(data, &messageConfig); err != nil { |
There was a problem hiding this comment.
I'm not sure I understand why we are marshalling and then immediately unmarshalling the data here.
| useTimeInterval: destination.useTimeInterval, | ||
| timeIntervalSeconds: destination.timeIntervalSeconds, | ||
| blockInterval: destination.blockInterval, | ||
| address: common.HexToAddress(destination.Address), |
There was a problem hiding this comment.
Can you confirm that destination.Address has already been verified so HexToAddress should never fail?
| addr = strings.TrimPrefix(addr, "0x") | ||
| _, err := hex.DecodeString(addr) | ||
| if err != nil { | ||
| return errors.Wrap(err, fmt.Sprintf("invalid address in block hash publisher configuration. Provided address: %s", destinationInfo.Address)) | ||
| } |
There was a problem hiding this comment.
Are we able to check that this is a valid address, and not just a valid hex string?
| if err != nil || intValue < 0 { | ||
| return 0, false, err | ||
| } |
There was a problem hiding this comment.
We should probably separate these cases. Right now if intValue < 0 we return a nil error.
| func (d *destinationSenderInfo) shouldSend(blockTimestamp uint64, blockNumber uint64) bool { | ||
| if d.useTimeInterval { | ||
| interval := d.timeIntervalSeconds | ||
| if time.Unix(int64(blockTimestamp), 0).Sub(time.Unix(int64(d.lastTimeSent), 0)) < (time.Duration(interval) * time.Second) { |
There was a problem hiding this comment.
Do we need to convert both of these to unix times?
| cmd := exec.Command( | ||
| "cast", | ||
| "send", | ||
| "--rpc-url", teleporterTestUtils.HttpToRPCURI(subnetInfo.ChainNodeURIs[0], subnetInfo.BlockchainID.String()), | ||
| "--private-key", hexutil.Encode(fundedKey.D.Bytes()), | ||
| "--create", blockHashReceiverByteCode, | ||
| ) |
There was a problem hiding this comment.
We should probably replace this with bind.DeployContract so we're not relying on cast. It also cuts out all the scanner boilderplate below.
| newHeads chan *types.Header, | ||
| subnetInfo teleporterTestUtils.SubnetTestInfo, | ||
| blockHashABI *abi.ABI, expectedHashes []common.Hash) { | ||
| newHead := <-newHeads |
There was a problem hiding this comment.
nit: Can we put a newline here? this is hard to read.
| @@ -9,12 +9,15 @@ type VM int | |||
| const ( | |||
| UNKNOWN_VM VM = iota | |||
| EVM | |||
| EVM_BLOCKHASH | |||
There was a problem hiding this comment.
I might be misunderstanding something, but this doesn't feel like it should be a distinct VM, I think it would make more sense as a flag? I also don't really understand how we're using it right now - Does an EVM_BLOCKHASH subscriber perform a superset of the actions performed byan EVM subscriber?
Why this should be merged
Adds relayer support for publishing verified block hashes from subnets. See ava-labs/subnet-evm#734 for more details on the
subnet-evmfeature and use cases. Also includes architecture and interface changes to better support one-to-many Warp message relaying for future anycast protocols.How this works
Adds
EVM_BLOCKHASHVM type, andBLOCK_HASH_PUBLISHERmessage protocol.EVM_BLOCKHASHsubscribes to all new blocks from the configured source, and passes it to the message relayer.BLOCK_HASH_PUBLISHERthen creates a single aggregate signature and publishes the block hash on each of the configured destinations viaBlockHashReceiver.sol, added here: ava-labs/teleporter#51.Some notes:
How this was tested
How is this documented
Code comments