[pleasew8t]

This PR brings a few hardening improvements to the Notary. The majority of changes relate to concurrency, where out of order execution (i.e, executing Run() while or after some other executes) could cause a panic. Note that the exact conditions where each of these instances would materialize were considered unlikely to occur, given how the Notary is used at present. But this may change in the future.

Summary of Changes

• Scope the Badger iterator with the "NOTARY" prefix to avoid full DB loads. Before this PR, the Notary would load the full DB, which had fairly significant resource consumption.
• Adds an atomic peek/pop function. In rare cases, a queue may be manipulated between when a peek and a pop occurs. The new PopReady() function allows doing both operations under the same mutex, accepting a time as input to assess whether or not a pending message is ready.
• Adds a few more nil checks that don't change the behaviour of the Notary. These are mostly for hardening.
• During construction, the Notary now instantiates blackholed, and during loadFromDB() it will only instantiate a new delayed queue and blackholed set if none exists. This fixes a race condition where, despite loadFromDB() locking a mutex, other functions would still read from these pointers. Unlikely to materialize, but easy to fix.
• Adds a ready flag to the Notary struct, which is used by public methods to exit early if the Notary is not ready yet.
• Ensures that the Run function is idempotent, by only allowing it to execute once. If it is ever necessary for multiple instances of the Notary to exist, this decision should be revisited.

Manual Review Notes

The commits have been structured to follow the changes listed above, so that reviewing each commit in order groups together changes linked to the same idea. The main thing to look out for is anything that actually changes the way the Notary operates (unrelated to lifecycle).

[djb15]

Thanks for the distinct commits, it made reviewing this PR really easy 🙏

Re the new ready checks, what are the scenarios in which the Notary might not have been ready? Also, what's the result now if the Notary isn't ready, do messages just go through (i.e. fail open)?

[pleasew8t]

Thanks for the distinct commits, it made reviewing this PR really easy 🙏

Re the new ready checks, what are the scenarios in which the Notary might not have been ready? Also, what's the result now if the Notary isn't ready, do messages just go through (i.e. fail open)?

The readiness flag is to ensure that you can't call Notary member functions without first initializing it through Run(). Prior to this PR, such ordering may have resulted in unwanted behaviour (including potential nil pointer dereferences), so this just closes that possibility. There isn't ever a scenario in which an uninitialized Notary would/should be used.

To answer your 2nd question - the behaviour is that it returns an error now, if not ready. The specific call sites that call into the Notary are already handling errors from the Notary, so this doesn't change that kind of behaviour.

[johnsaigle]

I think this PR fixes some known bugs, which is great 👍🏻

At the same time, IMO the implementation goes the wrong direction in other places.

The Notary has some consistency problems in it, including unclear ownership of locking, memory and the DB drifting apart on errors, and invalid states being representable. (Those exist in the current implementation too.)

Below are some things that IMO should be addressed. For clarity, this is a critique of my original implementation in addition to some of the changes in this PR. This can help the Notary implementation better embody what we've laid out in the Safety Critical Code doc.

• Who owns atomicity? Right now it's split between the queue's internal lock and the Notary's mutex, and some operations use one, some the other, and some neither (ReleaseReadyMessages, blackhole,RemoveBlackholedMsg). ProcessMsg is an important example: IsBlackholed, IsDelayed, and delay each lock separately, so an admin BlackholeDelayedMsg can land in between. A Valid reobservation then sees the message in neither place, gets Approve, and we sign something an admin just blackholed. I'd rather have one type own the queue, the blackhole set, and the DB under a single mutex, with multi-step transitions done under one lock.

• Memory and DB drift on errors. The remove helpers update memory before the DB. If the DB delete fails, the message is gone from memory but still on disk, and the next Valid reobservation gets approved. That fails open. On a DB error, memory should keep holding the message back.

• Invalid states are representable. The readiness flag means a Notary can exist without its state loaded, so every method needs an isReady check, and while it's false IsBlackholed just quietly returns false. If NewNotary loads state up front, that state can't exist and the flag goes away.

• PopReady invites the "unready" case. Having PopReady alongside a Pop that ignores ReleaseTime makes early release look like a normal operation. I think there should be one Pop(when) that checks readiness and removes under the lock, plus RemoveItem for targeting a specific message.

• Nil checks are inconsistent. Some queue methods tolerate a nil receiver and others don't. A nil msgPubSet behaves like an empty set, so Contains on a nil blackhole set says "not blackholed", which also fails open. The constructors always build these, so a nil is a bug and should just panic. A few other nil checks only exist because the mock DB returns nil for everything.

• The mock DB is too forgiving. The real DB errors when you delete a key that doesn't exist; the mock doesn't, so the tests can't catch bugs in the error paths.

Rather than block on the above, which could require a lot more discussion, I'll leave it up to @djb15 and @pleasew8t to figure out how much you want to change here. I've also opened #5095 to address all the above, and we can consider that separately.

[djb15]

@johnsaigle thanks for making the requested changes to the feedback. I've considered these in two buckets, where one set is directly relevant to the changes this PR introduced, and the other set is more improvements to the notary which we can review in the other PR. To summarize, I cherry-picked + ported the relevant commits from #5095 for the former bucket:

• Agree with the feedback on Pop.
• The feedback on the ready checking was also fair, and I also previously asked a question about it, so probably wasn't the neatest approach. Appreciate the alternative suggestion.
• Removed the redundant nil checks.