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 NotApplicable. 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, but tests only 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

djb15 · 2025-02-28T18:12:44Z

node/pkg/watchers/evm/watcher.go

+			switch w.env {
+			case common.UnsafeDevNet:
+				tbridge = eth_common.BytesToAddress(sdk.KnownDevnetTokenbridgeEmitters[w.chainID])
+				weth = eth_common.HexToAddress("0xDDb64fE46a91D46ee29420539FC25FD07c5FEa3E")


Re the TODO, how about creating a set of Known[X]NetWETHAddress constants in the SDK alongside the existing constants?

djb15 · 2025-02-28T18:13:46Z

node/pkg/watchers/evm/watcher.go

+			w.txVerifier, tvErr = txverifier.NewTransferVerifier(
+				w.ethConn,
+				&addrs,
+				20,


Comment to explain the choice of magic number?

… has

pleasew8t · 2025-03-04T10:47:55Z

sdk/mainnet_consts.go

+var KnownWrappedNativeAddress = map[vaa.ChainID]common.Address{
+	// WETH


It makes sense to try and maintain consistency here by typing the mapping as map[vaa.ChainID]string.

pleasew8t · 2025-03-04T10:48:51Z

sdk/devnet_consts.go

@@ -41,3 +42,9 @@ var KnownDevnetAutomaticRelayerEmitters = []struct {
 	{ChainId: vaa.ChainIDEthereum, Addr: "000000000000000000000000cc680d088586c09c3e0e099a676fa4b6e42467b4"},
 	{ChainId: vaa.ChainIDBSC, Addr: "000000000000000000000000cc680d088586c09c3e0e099a676fa4b6e42467b4"},
 }
+
+// KnownDevnetWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum
+var KnownDevnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{


Same comment here about typing to map[vaa.ChainID]string.

pleasew8t · 2025-03-04T10:49:18Z

sdk/testnet_consts.go

+var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{
+	// WETH


Same comment here about typing to map[vaa.ChainID]string. I'd also argue that we can have one less import in this file by doing so.

pleasew8t · 2025-03-04T11:06:19Z

node/pkg/common/chainlock.go

+// This type represents whether a message has been verified for some definition of verification. This may mean different things
+// on a per-application or per-chain basis.
+// NOTE: This status is currently used only by the Transfer Verifier for supported chains.


Suggested change

// This type represents whether a message has been verified for some definition of verification. This may mean different things

// on a per-application or per-chain basis.

// NOTE: This status is currently used only by the Transfer Verifier for supported chains.

// The `VerificationState` is the result of applying transfer verification to the transaction associated with the `MessagePublication`.

// While this could likely be extended to additional security controls in the future, it is only used for `txverifier` at present.

pleasew8t · 2025-03-04T11:07:19Z

node/pkg/common/chainlock.go

+
+const (
+	// The default state for a message. This can be used before verification occurs. If no verification is required, `NotApplicable` should be used instead.
+	// NOTE: this value is used as the default, zero-value for the type, so this field should not be re-ordered among other variants.


Can remove this line

pleasew8t · 2025-03-04T11:08:06Z

node/pkg/common/chainlock.go

+	Rejected
+	// Represents an unusual state after validation, neither confirmed to be good or bad.
+	Anomalous
+	// Represents a "known good" status where a Message has been validated and the result is good. The message should be process normally.


Suggested change

// Represents a "known good" status where a Message has been validated and the result is good. The message should be process normally.

// Represents a "known good" status where a Message has been validated and the result is good. The message should be processed normally.

pleasew8t · 2025-03-04T11:08:34Z

node/pkg/common/chainlock.go

@@ -33,6 +52,9 @@ type MessagePublication struct {
 	// Unreliable indicates if this message can be reobserved. If a message is considered unreliable it cannot be
 	// reobserved.
 	Unreliable bool
+	// This type represents whether a message has been verified for some definition of verification. This may mean different things


Reuse comment above VerificationState type definition.

pleasew8t · 2025-03-04T11:17:21Z

node/pkg/common/chainlock.go

 	)
 }
+
+func (v VerificationState) String() string {


Could move this definition up to the type definition.

pleasew8t · 2025-03-04T11:46:46Z

node/pkg/watchers/evm/watcher.go

+		}
+	}
+
+	updateErr := msg.SetVerificationState(verificationState)


I wonder about this pattern. My thinking is that before transfer verification this logic did not matter, and therefore the guardian would continue processing transfers regardless of the transfer verification operations. Should we not have a verification state that indicates this error ocurred, keep logging it, and pass the message to the msg channel anyway? Until we see this succeeding in a live environment, we should probably make transfer verification observent rather than interacting, if that makes sense.

I think that's what happens generally. This error is more of a type-safety thing to prevent overwriting an existing state. It could log an error instead of returning I suppose.

I moved this inside of the if statement in this function. In the previous commit, the state of the message would be changed from NotVerified to NotApplicable, and potentially return an error. Now, the error handling and state change will not occur unless transfer verifier is enabled.

pleasew8t · 2025-03-04T11:49:14Z

node/pkg/watchers/evm/watcher.go

@@ -701,14 +783,25 @@ func (w *Watcher) postMessage(logger *zap.Logger, ev *ethabi.AbiLogMessagePublis
 			zap.Uint8("ConsistencyLevel", ev.ConsistencyLevel),
 		)

-		w.msgC <- message
+		ctx, cancel := context.WithCancel(context.Background())


I took a queue from the Sui watcher and made it so the sui tx verifier uses context.WithTimeout, which kind of makes more sense to me. Unless there is something specific that cancels the context in the event of an error.

I think I chose WithCancel here because other parts of the watcher do this, and they can be cancelled if the code panics. Basically I'm going off of this:

wormhole/node/pkg/watchers/evm/watcher.go

Lines 206 to 210 in c913135

// later on we will spawn multiple go-routines through `RunWithScissors`, i.e. catching panics.

// If any of them panic, this function will return, causing this child context to be canceled

// such that the other go-routines can free up resources

ctx, watcherContextCancelFunc := context.WithCancel(parentCtx)

defer watcherContextCancelFunc()

pleasew8t · 2025-03-04T12:16:39Z

node/pkg/watchers/evm/reobserve.go

+
+			if pubErr != nil {
+				w.logger.Error("Error when publishing message", zap.Error(err))
+				continue


If we do decide to continue sending the message publication over the channel, regardless of potential runtime errors, remember to remove the continue so the number of observations is still incremented.

I removed the continue statements in the most recent commit

pleasew8t · 2025-03-04T12:18:16Z

devnet/node.yaml

@@ -146,6 +146,7 @@ spec:
            - --ccqEnabled=true
            - --ccqAllowedRequesters=beFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe,25021A4FCAf61F2EADC8202D3833Df48B2Fa0D54
            - --ccqAllowedPeers=12D3KooWSnju8zhywCYVi2JwTqky1sySPnmtYLsHHzc4WerMnDQH,12D3KooWM6WqedfR6ehtTd1y6rJu3ZUrEkTjcJJnJZYesjd89zj8
+            # - --transferVerifierEnabledChains=1


Is this meant to be commented?

Yes, similar to Governor in the same file.

pleasew8t · 2025-03-04T12:36:19Z

node/cmd/guardiand/node.go

+	// NOTE: Using a known capacity and counter here avoids unnecessary reallocations compared to using `append()`.
+	enabled := make([]vaa.ChainID, len(parsed))


Why not just append? This is startup, which I don't think is that performance-sensitive.

Agreed but I don't think there's much benefit to append

pleasew8t · 2025-03-04T12:41:13Z

node/cmd/guardiand/node.go

@@ -500,6 +510,8 @@ func init() {
 	gatewayRelayerKeyPassPhrase = NodeCmd.Flags().String("gatewayRelayerKeyPassPhrase", "", "Pass phrase used to unarmor the gateway relayer key file")

 	subscribeToVAAs = NodeCmd.Flags().Bool("subscribeToVAAs", false, "Guardiand should subscribe to incoming signed VAAs, set to true if running a public RPC node")
+
+	transferVerifierEnabledChains = NodeCmd.Flags().String("transferVerifierEnabledChains", "", "Transfer Verifier will be enabled for these chain IDs (comma-separated)")


Why not use Flags().IntSlice()?

Done using UintSlice in the latest commit

- Avoid modifying the message or changing error handling in the watcher's message publication flow when transfer verifier is disabled - Remove continue statements when verifyAndPublish returns an error in the reobservation flow - Modify code comments around VerificationState - Use UintSlice when parsing CLI flags - Change new SDK constants to be strings instead of common.Address to be consistent with other SDK values

djb15 · 2025-03-04T21:44:43Z

node/cmd/guardiand/node.go

@@ -500,6 +510,8 @@ func init() {
 	gatewayRelayerKeyPassPhrase = NodeCmd.Flags().String("gatewayRelayerKeyPassPhrase", "", "Pass phrase used to unarmor the gateway relayer key file")

 	subscribeToVAAs = NodeCmd.Flags().Bool("subscribeToVAAs", false, "Guardiand should subscribe to incoming signed VAAs, set to true if running a public RPC node")
+
+	transferVerifierEnabledChains = NodeCmd.Flags().UintSlice("transferVerifierEnabledChains", make([]uint, 2), "Transfer Verifier will be enabled for these chain IDs (comma-separated)")


What's the idea behind the default case?

Hm my intention is that you can invoke the guardian with e.g.:

txVerifierChains=1,2

but not txVerifierChains= (blank)

If you mean the make here, I gave it a length and capacity of 2 more or less arbitrarily. I guess I was thinking that if we enable Ethereum and Sui, that's two chains. The value could easily be 0 or 10 or anything else.

pleasew8t · 2025-03-05T03:19:23Z

sdk/mainnet_consts.go

+// KnownWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum
+var KnownWrappedNativeAddress = map[vaa.ChainID]string{
+	// WETH
+	vaa.ChainIDEthereum: "0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8",


Don't know what I'm missing, but WETH on eth mainnet is at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2

Yeah you're right, I think this was a bad copy-paste from the testnet or devnet constant

pleasew8t · 2025-03-05T03:39:41Z

node/pkg/watchers/evm/watcher.go

-		// Channel to send new messages to.
+		// Channel for sending new MesssagePublications. Messages should not be sent
+		// to this channel directly. Instead, they should be wrapped by
+		// a call to `publishIfSafe()`.


Update to verifyAndPublish()

pleasew8t · 2025-03-05T03:41:12Z

node/pkg/watchers/evm/watcher.go

+
+			// Fetch the constants for the Token Bridge and the WETH address from the SDK.
+			var tbridge []byte
+			var weth string


I think we should rename weth to wnative?

pleasew8t · 2025-03-05T03:52:37Z

node/pkg/watchers/evm/watcher.go

+				weth = sdk.KnownTestnetWrappedNativeAddresses[w.chainID]
+			case common.MainNet:
+				tbridge = sdk.KnownTokenbridgeEmitters[w.chainID]
+				// https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2


remove comment

pleasew8t · 2025-03-05T03:56:31Z

node/pkg/watchers/evm/watcher.go

+				CoreBridgeAddr:    w.contract,
+				TokenBridgeAddr:   eth_common.BytesToAddress(tbridge[:]),


In my code I added a KnownMainnetCoreContracts to the constants file. I think it just makes sense. You don't have to do it here, but I'm going to keep the change on my branch.

Why not use KnownTokenbridgeEmitters?

pleasew8t · 2025-03-05T03:57:16Z

node/pkg/watchers/evm/watcher.go

+			pruneHeightDelta := uint64(20)
+			var tvErr error


I can here Jeff and Bruce saying this needs to be a constant.

This needs to be a constant!

the prune height delta you mean?

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

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 added 12 commits March 3, 2025 14:59

Add transfer verification to the ethereum watcher

644e148

Update txverifier test to use the correct testnet token address

74da608

Modify txverifier API to be more convenient with the data the watcher…

65871b8

… has

fix formatting

53ebf74

Add chain config options for tx verifier

12879fb

Fix unused error and unused field

9a6d49f

Fix comments; change variable names for parse tx verifier chains

59908fa

Fix bug where txverifierEnabled was not set in NewEthWatcher

316b9ce

Add further error handling around setting up txverifier

f6a477a

Improve comments

ed4bb98

Handle the CLI flag more explicitly

3c4e0c6

Ensure that txVerifier CLI flag works as expected

44c4bbf

johnsaigle added 3 commits March 3, 2025 14:59

fix lint issues

bdc8169

node add verificationState to MessagePublication

4140da4

errcheck

bdcbf9f

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

pleasew8t reviewed Mar 4, 2025

View reviewed changes

djb15 reviewed Mar 4, 2025

View reviewed changes

pleasew8t reviewed Mar 5, 2025

View reviewed changes

		var KnownWrappedNativeAddress = map[vaa.ChainID]common.Address{
		// WETH

		var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{
		// WETH

	// Represents a "known good" status where a Message has been validated and the result is good. The message should be process normally.
	// Represents a "known good" status where a Message has been validated and the result is good. The message should be processed normally.

	// later on we will spawn multiple go-routines through `RunWithScissors`, i.e. catching panics.
	// If any of them panic, this function will return, causing this child context to be canceled
	// such that the other go-routines can free up resources
	ctx, watcherContextCancelFunc := context.WithCancel(parentCtx)
	defer watcherContextCancelFunc()

		// NOTE: Using a known capacity and counter here avoids unnecessary reallocations compared to using `append()`.
		enabled := make([]vaa.ChainID, len(parsed))

		CoreBridgeAddr: w.contract,
		TokenBridgeAddr: eth_common.BytesToAddress(tbridge[:]),

node: Integrate Transfer Verifier into the Ethereum watcher #4233

Are you sure you want to change the base?

node: Integrate Transfer Verifier into the Ethereum watcher #4233

Conversation

johnsaigle commented Jan 22, 2025 • edited Loading

New Features

If Transfer Verifier is enabled

If Transfer Verifier is not enabled

Design Considerations

Modifying MessagePublication

Scope of this PR

Questions

Related Work

banescusebi left a comment

Choose a reason for hiding this comment

djb15 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

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

johnsaigle Mar 4, 2025 • 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

johnsaigle commented Jan 22, 2025 •

edited

Loading

johnsaigle Mar 4, 2025 •

edited

Loading