node: Add Notary package for verifying MessagePublications #4315

johnsaigle · 2025-03-26T20:20:01Z

Overview

New Notary package

This PR adds a "Notary" package that can evaluates the verification state of a Message Publication. It provides a Verdict on what should occur, e.g. it reports that an Anomalous message should be delayed.

It stores messages that should be Delayed or Blackholed in a database (BadgerDB)

Details are in #4452

Changes to the Processor

The processor now inspects the Notary's queue of message to delay and skips publishing them until their ReleaseTime has passed. Once they are ready to release, it passes them to the Governor and Accountant as normal (assuming Guardians have them enabled).

Design decisions + supporting tasks

Database

The package uses the BadgerDB key-value store to delay and persist messages, similar to the Governor. This allows the Notary's decisions to persist across restarts.

An implementation is possible that would use only in-memory representations, but this has some drawbacks:

We would lose messages that are marked as delayed on a node restart
Messages with a Rejected status should never be processed until the end of time. We need a way to track this.

Rather than use the existing Database struct (like Governor and Accountant), a new NotaryDB struct was added that is a wrapper for the BadgerDB connection. The goal here is to isolate the Notary-related API from other modules.

Creation of min-heap data structure PendingMessageQueue

Conceptually, it makes sense to store delayed messages according to their release time. If the collection is always sorted, we can access elements from one side or the other until one message is too recent to process. In that case, all of the other messages will also be too new, and we can exit early.

This also allows us to be flexible with our delay time, compared with storing the time at which the message is stored rather than when it should be delayed. We can delay a message for one minute, a day, or a year. As long as the sorting holds, we don't need to iterate over many items.

In order to achieve this, I've added a min-heap data structure with a safe API that allows for storing messages this way.

Background

Follow-up task on: #4233

Future Work

Add drop/release/reset admin commands

This package should allow a super-minority of Guardians to forcibly drop, release, or reset the delay time of delayed or blackholed messages. This can help when the Notary has delayed an Anomalous message that is in fact fraudulent - in this case, the message can be moved to the 'blackhole' deny-list. Alternatively, if the Transfer Verifier or Notary has a bug, this allows the Guardians to release a harmless message.

Add Guardian p2p feature flags

Similar to: #4317

This change will be done in a follow-up PR.

Use the priority message queue min-heap for the Governor

The new data structure could be made generic or copied for the types used by the Governor and could grant similar benefits. See also #4029.

mdulin2 · 2025-04-10T22:19:19Z

node/pkg/notary/notary.go

+	Verdict uint8
+)
+
+const (


Does it make sense to have the terminology be similar to the transfer verification? I feel having "delay" and "anomalous" have similar purposes and "Rejected" and "Blackhole".

I disagree, I think verificaiton state is a status and the verdict is an action. For example, in the future we may want to Blackhole Anomalous messages instead. It's also possible that the Notary could be extended to give different verdicts on different types of messages.

node/pkg/notary/notary.go

mdulin2 · 2025-04-10T22:22:13Z

node/pkg/notary/notary.go

+		err = n.blackhole(msg)
+		v = Blackhole
+	case common.NotVerified, common.NotApplicable, common.Valid:
+		// NOTE: All other statuses are simply approved for now. In the future, it may be


If the message is being processed at this point, it should never be in this state, right? I feel like we should just bail if this happens.

For Valid and NotApplicable, this looks correct though.

What do you mean by bail? Do you want the function to return an error? My idea here was to fail open because ultimately we only care about Anomalous and Rejected right now.

NotVerified will be the status for all messages coming from chains without a transfer verifier so I think we want these to be approved as well.

mdulin2 · 2025-04-10T22:39:48Z

node/pkg/processor/processor.go

+
+				// Iterate over all ready messages. Hand-off to the Governor or the Accountant
+				// if they're enabled. If not, publish.
+				for msg := range slices.Values(p.notary.Ready()) {


It's repeated in three spots:

Here

msgC channel

Below for just the Governor.

I think a generalized function refactor to handle all of these cases would be good.

For what it's worth, I believe that the flow is correct in all of these cases. However, it's complicated and duplicates some code, which indicates that a refactor may be helpful.

Yeah I'm a bit nervous to dig into a big refactor of the processor, might want to do that in a follow-up PR instead.

node/pkg/processor/processor.go

mdulin2 · 2025-04-10T22:45:32Z

node/pkg/notary/notary.go

+	n.logger.Debug("notary: processing message", msg.ZapFields()...)
+
+	// Only token transfers are currently supported.
+	if !vaa.IsTransfer(msg.Payload) {


Is this necessary? I think that the case below for NotApplicable for probably handle this case right now?

Yeah it does, I was thinking this could be an extra guard in case the Transfer Verifier logic gets changed but the Notary gets forgotten.

node/pkg/notary/notary.go

mdulin2 · 2025-04-10T22:54:27Z

node/pkg/notary/notary.go

+	)
+	now := time.Now()
+	for n.delayed.Len() != 0 {
+		next := n.delayed.Peek()


I like using the heap for processing! It's clever.

Will this be compatible with when we want to move a message from delayed to Approved or Blackhole? I believe that's a future PR on the admin commands to handle these situations but just want to make sure we're thinking through the design of that now.

It'll go from Delayed to Approved automatically when the time runs out, but yeah the admin command will have to handle the case from moving manually from one place to the other.

I've added a RemoveItem function to the heap and added unit tests to show that it preserves the ordering of elements even if we remove an arbitrary one.

node/pkg/notary/notary.go

johnsaigle · 2025-05-01T21:30:10Z

Converting to draft because the behaviour of the Transfer Verifier is changing to add a new Verification State that will be used for the EVM implementation. Given this, we may want to change how the Processor responds to those messages.

#4362

johnsaigle · 2025-07-11T15:08:41Z

Note: I've moved the MessagePublication changes from this PR into a separate PR: #4428

Hopefully this makes things easier to digest and test. As a result, this PR is blocked but should be easily rebased after the other PR is merged.

djb15

First pass

djb15 · 2025-07-30T19:54:06Z

node/pkg/processor/processor.go

+			// This is the main message processing loop. It is responsible for handling messages that are
+			// received on the message channel. Depending on the configuration, a message may be processed
+			// by the Notary, the Governor, and/or the Accountant.
+			// This loop effectively causes each of these components to be process messages in a modular


Suggested change

// This loop effectively causes each of these components to be process messages in a modular

// This loop effectively causes each of these components to process messages in a modular

djb15 · 2025-07-30T20:06:18Z

node/pkg/common/pendingmessage.go

+	buf := new(bytes.Buffer)
+
+	// Compare with [PendingTransfer.Marshal].
+	//nolint:gosec // uint64 and int64 have the same number of bytes, and Unix time won't be negative.


nosec pls 🙏 Like the pattern of ignoring as little as possible

Thanks yeah this was authored before you left the similar comment on #4428 and i didn't come back and also apply that suggestion here. I'll fix this

djb15 · 2025-07-30T20:08:58Z

node/pkg/common/pendingmessage.go

+// UnmarshalBinary implements BinaryUnmarshaler for [PendingMessage].
+func (p *PendingMessage) UnmarshalBinary(data []byte) error {
+
+	if len(data) < marshaledMsgLenMin {


Should add 8 to account for the release time?

len(data) < marshaledMsgLenMin + 8

djb15 · 2025-07-30T20:12:29Z

node/pkg/common/pendingmessage.go

+// SECURITY:  Only the functions labelled Safe should be called directly. The other
+// functions must be public to satisfy the heap interface but do not perform
+// safety checks.
+// This follows the recommendations for using heap: heap.Pop() and heap.Push()
+// should be used rather than calling the below functions directly, and the
+// Safe functions do this.
+// See: https://pkg.go.dev/container/heap#Interface


I can't find any SafePush functions? Is this out of date?

It must be, I'll take a look and rework this comment. thanks

OK so in an earlier phase of this I was going to write a SafePush and SafePop in order to avoid the panics in the normal Push and Pop that you can see here.
Instead the approach I took is to make all the relevant methods private and wrap them in the PendingMessageQueue struct. That struct's Push and Pop methods do the relevant checks before calling the unsafe methods.

I updated the comment.

node/pkg/common/pendingmessage.go

djb15 · 2025-07-30T21:44:30Z

node/pkg/notary/notary.go

+	case common.Rejected:
+		err = n.blackhole(msg)
+		v = Blackhole
+	case common.CouldNotVerify, common.NotVerified, common.NotApplicable, common.Valid:


Worth splitting these into 2 sets and adding some debug logging for testnet?

Done, let me know what you think

djb15 · 2025-07-30T21:51:46Z

node/pkg/notary/notary.go

+		// Update database.
+		err := n.database.DeleteDelayed(pMsg)
+		if err != nil {
+			n.logger.Error("delete pending message from notary database", pMsg.Msg.ZapFields(zap.Error(err))...)


If this delete failed, the next time we release ready messages, the same message will be included again. Haven't checked yet myself, but would this have any downstream side effects of double processing?

This iterates over and consumes elements from the in-memory queue rather than the database. So, that situation would only happen when the Guardian restarts and it loads the delayed messages from the DB into the queue.

In general, if a message is processed more than once it would need to flow through the Governor and the Accountant successfully in order to be processed into a VAA. If a VAA has already been produced for that message, then making it all the way through those checks should result in a duplicated VAA.

We should definitely think through any new edge-cases that occur here, but with the caveat that the Governor and the Guardians themselves already do not have a permanent record of already-processed messages or VAAs, so I think this new code doesn't change that model.

djb15 · 2025-07-30T21:56:04Z

node/pkg/notary/notary.go

+	}
+
+	// Store in in-memory slice. This should happen even if a database error occurs.
+	n.delayed.Push(pMsg)


Should we be checking the database/heap first to see if the message has already been delayed? For example, we don't want the same message to get delayed twice, where one then gets manually blackholed, and then the other gets released as normal after the delay, effectively circumventing the blackhole

In the latest commit I added a uniqueness check in the queue's Push method so that duplicate MessagePublications can't be added.

djb15 · 2025-07-30T22:17:17Z

node/pkg/processor/processor.go

+					continue
+				case guardianNotary.Unknown:
+					p.logger.Error("notary returned Unknown verdict", k.ZapFields(zap.String("verdict", verdict.String()))...)
+				case guardianNotary.Approve:


Also add a default case that logs an error message? We shouldn't hit the default case so would be good to know if that happens

djb15 · 2025-07-30T22:21:09Z

node/pkg/processor/processor.go

+					if p.acct != nil {
+						shouldPub, err := p.acct.SubmitObservation(msg)
+						if err != nil {
+							return fmt.Errorf("accountant: failed to process message `%s`: %w", msg.MessageIDString(), err)


Do we want to return here? It'll mean we never get to the Governor pending messages?

This should mimic the existing logic for Accountant errors.
The Governor is already processed just above in this branch. The governor-related code below is what runs if the Notary is not enabled.
I don't like how this code is structured (hence the TODO above) but I didn't want to mess around with the processor logic too much.

…bridge validation - Replace TxID-based deduplication with MessageID for pending message queue - Add emitter address validation for token bridge transfers - Fix test helper functions to generate unique messages with proper sequence numbers - Add duplicate prevention tests and improve error handling

johnsaigle changed the title ~~Notary~~ node: Add Notary package for verifying MessagePublications Mar 26, 2025

johnsaigle added enhancement New feature or request node governor labels Mar 26, 2025

johnsaigle force-pushed the notary branch 4 times, most recently from cdbdccd to 43b0fa5 Compare April 10, 2025 16:04

johnsaigle marked this pull request as ready for review April 10, 2025 18:34

johnsaigle requested review from bruce-riley, evan-gray, panoel, SEJeff, djb15, mdulin2 and pleasew8t as code owners April 10, 2025 18:34

mdulin2 requested changes Apr 11, 2025

View reviewed changes

johnsaigle marked this pull request as draft May 1, 2025 21:28

johnsaigle force-pushed the notary branch from c5b5423 to 5c51209 Compare May 6, 2025 19:44

johnsaigle force-pushed the notary branch from c0f6427 to 60a4520 Compare July 11, 2025 15:09

johnsaigle mentioned this pull request Jul 11, 2025

node: Fix marshaling for MessagePublications #4428

Merged

johnsaigle added 7 commits July 28, 2025 09:48

node: Add Notary package for verifying MessagePublications

6c48a23

Remove parsing of old transfers from the governor

f8571ce

revert changes to governor code

92ffdeb

Remove relative dir replace directives in go.mod

7abe557

add custom errors; use nil in constructor

44d23f3

Add unit tests for marshaling and heap-ordering

9ee0166

Be consistent with naming and update interface

70a0545

johnsaigle added 22 commits July 28, 2025 09:50

Properly enable notary in processor; add more logging

fdeb6f0

Add unit tests and mocking for the notary

29cd057

Change serialization of msg pub timestamp to uint64

11f1f15

Fix serialization for timestamp

952ee7c

Add unit tests and fix bugs

1095a4e

Address lint errors

c8353ff

Add test for collections updating

c935939

Add happy-path database tests

ddd1e13

add more unit tests

82a30e7

Fix missing argument for interface

209d89e

fix formatting

8a615fe

fix new lints added in golangci-lint v2

96c98ce

Add notary codeowners

706dda1

rework notary implementation

3d2fc0c

address notes from code review

e926451

add unit tests for remove and ordering

fd4ebb7

fix lint errors

3799ce8

Fix Notary codeowner

be48ee6

update marshaling

7a09710

Add CouldNotVerify handling to notary

3151150

improve commenting around the main processor arms

320e596

small changes after rebase

ad3c9d6

johnsaigle force-pushed the notary branch from 530f26a to ad3c9d6 Compare July 28, 2025 14:01

Add test coverage for new VerificationState variant; add nil checks

e8aa7a9

johnsaigle marked this pull request as ready for review July 28, 2025 15:25

Make delay and blackhole mutually exclusive; add tests

97cc425

johnsaigle mentioned this pull request Jul 29, 2025

docs: Add whitepaper for Notary #4452

Open

djb15 reviewed Jul 30, 2025

View reviewed changes

johnsaigle added 2 commits July 31, 2025 09:31

Address comments from code review

a17a7dc

	// This loop effectively causes each of these components to be process messages in a modular
	// This loop effectively causes each of these components to process messages in a modular

node: Add Notary package for verifying MessagePublications #4315

Are you sure you want to change the base?

node: Add Notary package for verifying MessagePublications #4315

Conversation

johnsaigle commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

New Notary package

Changes to the Processor

Design decisions + supporting tasks

Database

Creation of min-heap data structure PendingMessageQueue

Background

Future Work

Add drop/release/reset admin commands

Add Guardian p2p feature flags

Use the priority message queue min-heap for the Governor

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsaigle Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

johnsaigle commented May 1, 2025

Uh oh!

johnsaigle commented Jul 11, 2025

Uh oh!

djb15 left a comment

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!

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

johnsaigle commented Mar 26, 2025 •

edited

Loading

johnsaigle Apr 11, 2025 •

edited

Loading