node: Update accountant audit logic by djb15 · Pull Request #4947 · wormhole-foundation/wormhole

djb15 · 2026-08-17T21:18:02Z

Observation batches are now packed against the exact serialised transaction size rather than always batching up to a maximum batch size of 100, deferring messages that don't fit to a later batch. A message too large to ever fit is submitted in its own batch, with a new metric and associated log.

djb15 · 2026-08-17T21:18:18Z

@claude

github-actions · 2026-08-17T21:18:39Z

Claude finished @djb15's task in 4m 39s —— View job

Code Review: Update accountant audit logic

Gather context and read changed files
Review metrics.go changes
Review submit_obs.go changes
Review submit_obs_test.go changes
Post review findings

Overall: This PR looks good. The implementation is well-structured, the size model is correct, and the tests are thorough. No blocking issues found.

Summary

The PR replaces the previous fixed-count batching (up to 100 observations per batch, regardless of serialized size) with size-aware packing that ensures each batch's marshaled submit_observations message stays within the 64KB wasm contract input limit. Messages that don't fit in the current batch are deferred to a later one; messages too large to ever fit are isolated in their own batch and submitted anyway (in case the size estimate is conservative), with appropriate logging and metrics.

What I verified

Size model correctness: I traced the size computation end-to-end:

marshaledObservationSize (submit_obs.go:149) marshals each Observation to JSON and returns its byte length. This correctly accounts for vaa.Address's custom MarshalJSON (hex string, fixed 66 bytes including quotes — verified at sdk/vaa/structs.go:132-134) and []byte fields (base64-encoded by encoding/json).
submitObservationsMsgOverhead (submit_obs.go:174-190) is computed at init time by marshaling a SubmitObservationsMsg with worst-case field values (max uint32 for guardian set index and signature index, all-0xFF signature bytes) and empty observations, then subtracting the 2-byte "" from the empty observations string. This gives the exact overhead excluding the observations content.
submitObservationsMsgSize (submit_obs.go:167-169) adds the overhead + quotes + base64.StdEncoding.EncodedLen(obsArraySize). This matches how SubmitObservationsToContract works: the observations array is marshaled to JSON bytes, assigned to Observations []byte, and the outer message is marshaled — encoding/json base64-encodes the []byte field using base64.StdEncoding.
The JSON array size tracking in packObservationBatches correctly models [obs1,obs2,...,obsN] as brackets + obs1 + comma + obs2 + ... + comma + obsN.
TestSubmitObservationsMsgSizeMatchesMarshaledMsg (submit_obs_test.go:132-147) validates that the computed size exactly matches actual marshaling, serving as a strong regression guard.

Bin packing algorithm: First-fit packing (submit_obs.go:122-146). For each message, it tries existing batches in order and places the message in the first batch with room (both count ≤ 100 and size ≤ 64KB). If none fit, a new batch is created. Oversized messages (too large even alone) are flagged but still submitted in their own batch. Message ordering within each batch is preserved. The algorithm is O(n·b) where b is the number of batches — bounded and efficient for realistic inputs.

Refactoring: The makeObservation helper (submit_obs.go:281-292) correctly deduplicates the Observation construction that previously lived inline in SubmitObservationsToContract. The extracted code is identical to the original (verified by diff).

Metrics: Two new Prometheus counters (metrics.go:65-74) — batchSizeSplits and oversizedObservations — follow existing naming conventions and are incremented at the appropriate points.

Tests: The test suite (submit_obs_test.go:149-246) covers: single batch, count-limited splitting, size-limited splitting, deferred large messages, and oversized message isolation. The shared invariants check (every message in exactly one batch, count/size limits respected) is a strong correctness guard.

Findings

Two low-severity inline comments posted — no blocking issues.

johnsaigle · 2026-08-18T10:28:02Z

+		batchSizeSplits.Inc()
+	}
+	for _, msg := range oversized {
+		// The message is submitted anyway (alone, so it can't take any other observations down with it) in case our size accounting is too conservative.


Should we add a log in the case where an oversized message does not fail? This would help give us a signal to change the limit.

Also do they still get submitted? The loop below seems to only submit messages under batches, not oversized

johnsaigle · 2026-08-18T11:05:57Z


 const (
-	DefaultSubmitObservationBatchSize = 100             // Observations per batch (limited by wasm contract input size of 64KB)
+	DefaultSubmitObservationBatchSize = 100             // Maximum observations per batch, also subject to maxSubmitObservationsMsgSize


Could you change this to be uint8 (rather than the default int)?

We don't use the non default int type for constants in most other places in the Guardian, why the ask to change that?

johnsaigle · 2026-08-18T11:06:11Z

 const (
-	DefaultSubmitObservationBatchSize = 100             // Observations per batch (limited by wasm contract input size of 64KB)
+	DefaultSubmitObservationBatchSize = 100             // Maximum observations per batch, also subject to maxSubmitObservationsMsgSize
+	maxSubmitObservationsMsgSize      = 64 * 1024       // Maximum size of the marshaled submit_observations message (the wasm contract input is limited to 64KB)


Could you change this to be e.g. uint16?

johnsaigle · 2026-08-18T11:07:11Z

+// so a large message that does not fit in the current batch is deferred to a later batch rather than failing the messages
+// around it. A message too large to fit even in a batch by itself is returned in oversized as well as being placed in its own
+// batch.
+func packObservationBatches(msgs []*common.MessagePublication, maxBatchCount int, maxMsgSize int) (batches [][]*common.MessagePublication, oversized []*common.MessagePublication) {


Suggested change

func packObservationBatches(msgs []*common.MessagePublication, maxBatchCount int, maxMsgSize int) (batches [][]*common.MessagePublication, oversized []*common.MessagePublication) {

func packObservationBatches(msgs []*common.MessagePublication, maxBatchCount uint8, maxMsgSize uint16) (batches [][]*common.MessagePublication, oversized []*common.MessagePublication) {

We can use stricter types here to make it impossible to accidentally call these with negative values, or values that would be too big to make sense.

johnsaigle · 2026-08-18T11:07:41Z

+// batch.
+func packObservationBatches(msgs []*common.MessagePublication, maxBatchCount int, maxMsgSize int) (batches [][]*common.MessagePublication, oversized []*common.MessagePublication) {
+	// batchSizes[i] is the size the marshaled observations array for batches[i] would have, including the enclosing brackets.
+	var batchSizes []int


Suggested change

var batchSizes []int

var batchSizes []uint16

johnsaigle · 2026-08-18T11:12:11Z

+	return submitObservationsMsgOverhead + jsonQuotesSize + base64.StdEncoding.EncodedLen(obsArraySize)
+}
+
+// submitObservationsMsgOverhead is the number of bytes in a marshaled submit_observations message excluding the base64-encoded


Is this value a constant ultimately? Maybe we could pre-compute the value and use unit tests to lock it in.

johnsaigle · 2026-08-18T11:13:15Z

+func makeObservation(msg *common.MessagePublication) Observation {
+	return Observation{
+		TxHash:           msg.TxID,
+		Timestamp:        uint32(msg.Timestamp.Unix()), // #nosec G115 -- This conversion is safe until year 2106


Would be nice to use the SDK's TimeFromUnix here instead of fighting the linter.

I need to get those CodeQL rules going!!

johnsaigle · 2026-08-18T11:14:33Z

+	t.Helper()
+	emitterAddr, err := vaa.StringToAddress("0x0290fb167208af455bb137780163b7b7a9a10c16")
+	require.NoError(t, err)
+	return &common.MessagePublication{


Just for completeness it would be nice to add all of the fields here, i.e. IsReobservation, Unreliable, VerificationState

johnsaigle · 2026-08-18T11:15:07Z

+
+// marshaledMsgSizeForBatch builds the submit_observations message for a batch the same way SubmitObservationsToContract does,
+// with worst-case values for the fields other than the observations, and returns its marshaled size.
+func marshaledMsgSizeForBatch(t *testing.T, batch []*common.MessagePublication) int {


It might be better to not use int here as the return type

Vs a smaller type it's practically impossible for us to overflow an int. And based on the logic in the function it will never be negative so I'm not sure why we should go more defensive here?

johnsaigle · 2026-08-18T11:15:57Z

+}
+
+// marshaledMsgSizeForBatch builds the submit_observations message for a batch the same way SubmitObservationsToContract does,
+// with worst-case values for the fields other than the observations, and returns its marshaled size.


Is this a constant value we could save instead of using a function?

johnsaigle · 2026-08-18T11:26:36Z

+func packObservationBatches(msgs []*common.MessagePublication, maxBatchCount int, maxMsgSize int) (batches [][]*common.MessagePublication, oversized []*common.MessagePublication) {
+	// batchSizes[i] is the size the marshaled observations array for batches[i] would have, including the enclosing brackets.
+	var batchSizes []int
+	for _, msg := range msgs {


Let's add a nil check here

node: Update accountant audit logic

d9d965d

github-actions Bot reviewed Aug 17, 2026

View reviewed changes

Comment thread node/pkg/accountant/submit_obs.go Outdated

github-actions Bot reviewed Aug 17, 2026

View reviewed changes

Comment thread node/pkg/accountant/submit_obs_test.go

Claude PR feedback

81485f3

djb15 marked this pull request as ready for review August 17, 2026 21:41

djb15 requested review from SEJeff, bemic, evan-gray, johnsaigle, kcsongor, mdulin2, panoel and pleasew8t as code owners August 17, 2026 21:41

johnsaigle reviewed Aug 18, 2026

View reviewed changes

	func packObservationBatches(msgs []common.MessagePublication, maxBatchCount int, maxMsgSize int) (batches [][]common.MessagePublication, oversized []*common.MessagePublication) {
	func packObservationBatches(msgs []common.MessagePublication, maxBatchCount uint8, maxMsgSize uint16) (batches [][]common.MessagePublication, oversized []*common.MessagePublication) {

Uh oh!

Conversation

djb15 commented Aug 17, 2026

Uh oh!

djb15 commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: Update accountant audit logic

Summary

What I verified

Findings

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 17, 2026 •

edited

Loading