Skip to content

node: Accountant audit improvements#4839

Open
djb15 wants to merge 6 commits into
wormhole-foundation:mainfrom
djb15:accountant-improvements
Open

node: Accountant audit improvements#4839
djb15 wants to merge 6 commits into
wormhole-foundation:mainfrom
djb15:accountant-improvements

Conversation

@djb15

@djb15 djb15 commented May 26, 2026

Copy link
Copy Markdown
Collaborator

This PR includes the following changes to the Accountant:

  • Increases the max batch size of observation submissions from 10 to 100. This is close to the maximum calldata that the contract can support. To support this, there is an increase in the gas limit to 5m.
  • Increase the batch time from 100ms to 2s. Block time on Wormchain is ~6 seconds so there is little benefit to batching earlier, especially considering observation submission wait for block inclusion.
  • The Accountant audit flow has been modified to instead call query_all_pending_transfers (rather than query_missing_observations), as the former supports pagination. This guarantees that the audit flow will always run and there is no risk of exceeding query gas limits.
  • New tests are added to verify the changes perform as expected. The changes have also been tested on devnet and appear to work as expected.
  • Submitting to the Accountant subChan now has a blocking variant. When submitting to the channel from the audit flow we will happily wait for room to open up in the channel, vs from the processor we continue to try and move on if there is a failure.
  • Manual reobservation requests will trigger a retry on the Accountant if it is stuck in the pending state of an audit. This ensures that specific transactions can be posted to the contract despite being in an audit queue.

Other avenues explored:

  • I explored increase the number of messages per transaction to increase the effective batch size about 100. However submissions then use too much gas and we run into block gas constraints. With the limit of 5m per batch we could still have 95m gas per block (considering 19 guardians all submitting at the same time). We can't increase this any further.
  • I explored moving away from waiting for block inclusion to submitting transactions asynchronously. This could allow more transactions to land in each block from each guardian. However we still run into the same block gas constraints and the logic becomes more complicated as we need to track account sequence numbers manually (they are only incremented after inclusion of a transaction in a block) and retries are more challenging.
  • I explored modifying the accountant smart contract do add pagination for query_missing_observations, however this would require a smart contract upgrade. I elected to instead stick to making only Guardian changes for simplicity and ease of release.

@djb15

djb15 commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

@claude

@github-actions

github-actions Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @djb15's task in 3m 30s —— View job


Code Review: Accountant Audit Improvements

  • Read repository guidelines
  • Gather full diff and understand changes
  • Review accountant.go changes
  • Review audit.go changes
  • Review submit_obs.go changes
  • Review watcher.go changes
  • Review send_tx.go changes
  • Review metrics.go changes
  • Review test files
  • Post review findings

Summary

This PR makes substantial improvements to the accountant audit flow — switching from query_missing_observations (which lacks pagination) to query_all_pending_transfers (which supports pagination), increasing batch sizes and timeouts, and adding a blocking channel write mode for the audit path. The approach is well-reasoned and the test coverage for the new functionality is good.

I've posted 6 inline comments. Here's a summary by severity:

High (1)

Finding File Line
Missing guardian set index filter in performAudit audit.go L265

The file prologue states "If the transfer is for a different guardian set, it is skipped", but this filter is not implemented. The code iterates all data entries regardless of GuardianSetIndex. During guardian set transitions, guardianIndex derived from the current set is used to check hasGuardianSigned against signature bitmasks from a different set, where the same index may correspond to a different guardian. This can cause missed signatures or unnecessary resubmissions.

Medium (2)

Finding File Line
State lock held during 30s blocking write accountant.go L450-472
No upper bound on pagination loop audit.go L419-441

Low (3)

Finding File Line
delete called redundantly inside inner loop audit.go L286-287
Log count uses map keys instead of total entries audit.go L160
hasGuardianSigned silently returns false on parse failure audit.go L135

What looks good

  • The security comment at audit.go L255-261 explicitly documenting the trust boundary between knownPendingTransferMap (locally verified) and contract response data (untrusted) is excellent.
  • Pagination implementation with cursor-based start_after is clean and well-tested.
  • The hasGuardianSigned bitmask logic is correct and has thorough test coverage including edge cases (large values, invalid input, all-guardians-signed).
  • The blocking vs non-blocking channel write separation is well-designed — processor path stays non-blocking while audit can afford to wait.
  • Test coverage is comprehensive: audit phase 1, phase 2 (committed, unknown, digest mismatch), query errors, pagination, and signature bitmask parsing.

Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/audit.go Outdated
Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/accountant.go
Comment thread node/pkg/accountant/audit.go Outdated
Comment thread node/pkg/accountant/audit.go
johnsaigle
johnsaigle previously approved these changes May 27, 2026

@johnsaigle johnsaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small wording suggestion, feel free to ignore.

Comment thread node/pkg/accountant/audit.go Outdated
johnsaigle
johnsaigle previously approved these changes May 27, 2026
Comment thread node/pkg/accountant/audit.go Outdated
Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/accountant.go Outdated
Comment thread node/pkg/accountant/audit.go Outdated
pleasew8t
pleasew8t previously approved these changes Jun 15, 2026
Comment thread node/pkg/accountant/accountant.go
Comment thread node/pkg/accountant/accountant.go
Comment thread node/pkg/accountant/accountant.go Outdated
Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/audit.go
Comment thread node/pkg/accountant/submit_obs.go Outdated
Comment thread node/pkg/accountant/audit.go
@djb15 djb15 dismissed stale reviews from pleasew8t and johnsaigle via 5419622 June 30, 2026 21:43
@djb15 djb15 force-pushed the accountant-improvements branch from f9b65af to 5419622 Compare June 30, 2026 21:43
@djb15 djb15 requested a review from mdulin2 June 30, 2026 21:47
@djb15 djb15 requested review from bemic, johnsaigle and pleasew8t June 30, 2026 21:47
@djb15 djb15 force-pushed the accountant-improvements branch from efaf89b to e0c64d8 Compare June 30, 2026 23:35
@johnsaigle johnsaigle force-pushed the accountant-improvements branch from e0c64d8 to 113c69b Compare July 6, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants