node: Integrate Transfer Verifier into the Ethereum watcher #4233

johnsaigle · 2025-01-22T18:46:11Z

New Features

Create a TxVerifier instance in the Run function of the EVM watcher
Wrap all instances where a message would be published to the broadcasting msg channel with a new function, verifyAndPublish
Messages are published with a new status: VerificationState.
Modified the Transfer Verifier pkg API so that we can call the method with either an existing Receipt or a txHash.
Added unit test in the watcher for new functionality

If Transfer Verifier is enabled

Messages that are token transfers will undergo Transfer Verification
Message will be published with a status of Valid or Rejected depending on the result
The calling code can then decide what to do based on this status

If Transfer Verifier is not enabled

Existing behaviour will be preserved, but messages will be published with a status of NotVerified. No further actions are taken when a Message Publication has this status

Design Considerations

Modifying MessagePublication

This PR modifies MessagePublication to add a new status based on whether the Message Publication is verified. This decision was made to handle Transfer Verification cases across many chains. For example, the EVM logs are reliable enough that we can confidently rule a message as Valid or Rejected. Other ecosystems (i.e. Sui, but perhaps also Solana, etc.) are not so clear cut. In this case, we may want to mark a transaction as Anomalous rather than outright rejecting it.

Using this new enum allow us to do this.

Other potential benefits:

Decouples the Verification of a Message from whether or not this can be published.
Avoids scope-creep for the Watchers: they only watch messages, but do not need to reject them. (Instead this could be handled by the processor or some other security mechanism akin to the Governor or Accountant.)
Allows configuring targeted action on a per-chain and per-status basis. For example, we may want to delay Anomalous messages but drop Rejected ones.
Preserves a NotApplicable state that can be used as a fallback mechanism if the Transfer Verifier is disabled outright or on a particular chain
This status could be used in other cases beyond Transfer Verification, but should not interfere with existing message handling.

Scope of this PR

The idea with this PR is to test the modifications to the Ethereum watcher without enabling the transfer verifier yet. We can release this in testnet or mainnet to be sure that the changes here are stable.

Once we're confident that the underlying mechanism is working well, we can add some logic in the Guardian to actually react when a MessagePublication has a status like Rejected or Anomalous. For now, changing the reaction of the watcher to a "bad" message is out of scope.

Questions

Is it necessary to update the Protobuf files somewhere in order to capture the changes to Message Publication?

Related Work

Note: txverifier: fix bugs and improve error handling #4277 should be merged before this PR.
txverifier: Increase integration test coverage and simplify Tilt set-up #4280 extends the integration test coverage of the underlying package

banescusebi

Just a couple of suggestions

node/pkg/watchers/evm/watcher.go

node/cmd/guardiand/node.go

node/pkg/watchers/evm/watcher.go

djb15

Integration looks good overall! Would be good to deal with the outstanding TODOs if possible

node/pkg/watchers/evm/watcher.go

sdk/mainnet_consts.go

sdk/devnet_consts.go

sdk/testnet_consts.go

node/pkg/common/chainlock.go

node/pkg/txverifier/evm.go

node/pkg/watchers/evm/watcher.go

bruce-riley

Left a couple minor comments and one more major one that I think we should be pegging prom metrics or something when verification fails. Otherwise how will anyone know it's happening?

bruce-riley · 2025-03-19T16:58:36Z

Left a couple minor comments and one more major one that I think we should be pegging prom metrics or something when verification fails. Otherwise how will anyone know it's happening?

@johnsaigle and I talked offline and agreed that adding a metric could be handled in a follow-on PR.

djb15

Minor comment, looks good otherwise. I assume this is going to be tested in testnet/devnet first with some token transfers and performance evaluated?

node/pkg/watchers/evm/watcher.go

johnsaigle · 2025-03-20T12:57:33Z

@djb15 Yeah I think we can spend some time testing and benchmarking before the feature actually gets enabled by the Guardians.

evan-gray

Only reviewed devnet as that was the only change missing code owner approval.

johnsaigle added node evm labels Jan 22, 2025

banescusebi reviewed Jan 23, 2025

View reviewed changes

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

djb15 reviewed Jan 23, 2025

View reviewed changes

node/cmd/guardiand/node.go Outdated Show resolved Hide resolved

johnsaigle force-pushed the tv-eth-watcher branch 3 times, most recently from 688ee88 to 96a2566 Compare February 27, 2025 20:36

pleasew8t reviewed Feb 28, 2025

View reviewed changes

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

pleasew8t reviewed Feb 28, 2025

View reviewed changes

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

johnsaigle marked this pull request as ready for review February 28, 2025 15:58

johnsaigle requested review from bruce-riley, panoel, evan-gray, mdulin2 and SEJeff as code owners February 28, 2025 15:58

djb15 reviewed Feb 28, 2025

View reviewed changes

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

node/pkg/watchers/evm/watcher.go Outdated Show resolved Hide resolved

johnsaigle requested a review from kev1n-peters as a code owner February 28, 2025 20:37

johnsaigle marked this pull request as draft March 3, 2025 18:02

johnsaigle force-pushed the tv-eth-watcher branch from 2e9b737 to bdcbf9f Compare March 3, 2025 19:59

johnsaigle marked this pull request as ready for review March 3, 2025 20:31

johnsaigle requested review from pleasew8t and djb15 March 3, 2025 20:31

johnsaigle marked this pull request as draft March 3, 2025 20:35