[djb15]

This PR introduces a set of tests that verify the normal observation path will always lead to the same hash as the reobservation path for the EVM watcher.

It includes some refactor to the watcher to aid in testing which should be reviewed carefully to ensure that the current behaviour hasn't been changed inadvertently.

[djb15]

@claude

[github-actions]

Claude finished @djb15's task in 5m 47s —— View job

---

Code Review: EVM Watcher Divergence Tests (PR #4931)

• Gather context and read CLAUDE.md
• Review watcher.go refactoring
• Review by_transaction.go changes
• Review reobserve.go changes
• Review new test files
• Review test data and helpers
• Post final review

---

Summary

This is a well-structured PR that accomplishes two things: (1) refactors the EVM watcher's Run() method by extracting goroutine loop bodies into named methods, and (2) adds comprehensive divergence tests that verify the observation and reobservation paths produce identical message digests.

Verdict: Approve (no blocking issues found)

Production Code Changes

watcher.go refactoring — The five extracted methods (runGuardianSetPoller, runDelegatedGuardianConfigPoller, runReobservationHandler, runMessageProcessor, runHeaderProcessor) are mechanically identical to the original inline code. I verified each against the pre-PR version. The extracted methods correctly use w.logger (which is the same value as the local logger variable assigned at watcher.go:263). Error channel (errC) propagation is preserved where needed and correctly omitted from runReobservationHandler (which never used it).

postMessage split into publishMessageImmediately + queuePendingMessage — Clean decomposition. The defense-in-depth receipt validation in publishMessageImmediately is preserved. The old code had two separate log messages for the two receipt failure modes; the new code uses validateTransactionReceipt which encodes the failure type in the error, so diagnostic information is preserved via zap.Error(valErr).

newMessagePublication extraction (by_transaction.go) — This is the key safety improvement. Both the live-observation path (postMessage) and the reobservation path (MessageEventsForTransaction) now construct MessagePublication objects through a single function, eliminating the risk of field-mapping divergence. The IsReobservation flag is set at construction (via the isReobservation parameter) rather than mutated afterward in reobserve.go, which is a cleaner pattern.

validateTransactionReceipt extraction (by_transaction.go) — Shared validation for receipt nil-check and status-check, used from three call sites. The security comment accurately describes the rationale. One minor code quality note posted as an inline comment.

isReobservation parameter on MessageEventsForTransaction — All call sites updated correctly: reobserve.go passes true, parse_eth_tx.go passes false, tron_integration_test.go passes false.

Test Additions

Observation/Reobservation Parity Test — The TestObservationReobservationParity test drives both the live-observation path (via runMessageProcessor) and the reobservation path (via runReobservationHandler) against 410+ receipt fixtures. It asserts the digest multisets match and pin to independently-computed hashes. The test design using digestMultiset for order-independent comparison is appropriate since the two paths may emit messages in different orders.

Generated Receipt Golden Vectors — 210 synthetic vectors covering edge cases (empty/binary/all-zero payloads, leading-zero emitters, uint64-max sequences, high-bit timestamps, multiple Wormhole events per receipt, non-standard chain IDs, all consistency levels). The coverage test (TestGeneratedReceiptGoldenVectorsCoverage) pins these properties so fixture regeneration cannot silently weaken the corpus.

Metadata Independence Test — TestGeneratedReceiptGoldenVectorMetadataIndependence verifies that receipt metadata (tx hash, block hash, block number, gas fields, bloom, log indices) do not affect the digest. This is important because these fields are in the receipt but must not be part of the signed VAA body.

Test Helpers — newTestWatcher, mockConnector, fakeSubscription, seedReceipt, recvMsg with timeout — all well-designed for the test scenarios. The recvMsg helper with a 2-second timeout prevents test hangs.

Findings

Two low-severity inline comments posted:

1. by_transaction.go:58 — validateTransactionReceipt wraps nil error with %w when receipt == nil && err == nil (pre-existing, cosmetic)
2. observation_reobservation_parity_test.go:148 — require.Eventually format args evaluated eagerly (diagnostic quality only)

No security, correctness, or performance issues identified.

---

[johnsaigle]

Some initial thoughts on the PR, mostly suggestions.
I'll take a deeper look at this soon but I wanted to provide some of the feedback I have so far.

[johnsaigle]

I think the PR is in good shape. The testing strategy should help catch regressions which is great.

Overall I'm in support of the refactoring. It's nice to have the message publication be built only in a single place as well as to split out some of the more complicated functionality into separate functions. This facilitates testing.

A few things to note:

• For safety we probably want to run this on devnet/testnet for a while just to make sure the watcher changes are sound
• The test cases are generated from Ethereum mainnet. We could consider adding real receipts from other chains too. This would be a separate PR though.
• I double-checked the goroutine refactoring parts and I believe the new functions containing them are exactly the same as the originals
• The script to generate the test cases isn't included here. We could consider adding this in a follow-up PR in case we ever want to regenerate the test cases.

[pleasew8t]

I did a quick investigation into this, since the transaction status is verified earlier during evm-require-successful-receipt-before-observation. I don't understand the rule at all, but some guided work tells me that the CodeQL rule needs to include context of the validateTransactionReceipt function.

Additionally, isSafeMessageEventsFunction only checks MessageEventsForTransaction, and not MessageEventsForTransactionReobservation.

The CodeQL rule will need to be updated to support the refactor (cc @djb15 @bemic).