feat: store discovered handshakes as facts#24483
Conversation
…f-766-handshake-facts
…d re-pin standard contracts
23d58bd to
5fd8fd3
Compare
| /// Versions originated at or before the finalized tip can never be retracted, so anything older than the newest | ||
| /// finalized version can never become current again. [`write`](Self::write) discards that dead history. After the | ||
| /// first finalized write, a reorg can therefore roll the register back at worst to that finalized version. | ||
| pub struct RetractableRegister<T> { |
There was a problem hiding this comment.
Our sync cursor is basically a block number that we use to determine how to sync (we fetch logs from cursor to anchor block). This happens each time we execute a sync on a contract, so it can be quite a frequent. So if we used "plain" facts, we would be creating lots of them.
But in this case, we only care about the "last version" of the sync cursor. Once we have a finalized version, we can clean all that came before. I thought this could happen in other cases, so I tried to generalize it a little bit here
I don't love the name RetractableRegister, so I'm open to suggestions
There was a problem hiding this comment.
Maybe VersionedRecord? I think the main salient feature of this is the idea of multiple versions of the same thing
There was a problem hiding this comment.
Do we need it to be pub? I would keep it pub(crate) if possible so we don't buy into a backwards compatibility commitment for something that is new (this might as a consequence call for this module to be moved to where the handshake registry contract lives, I don't know)
There was a problem hiding this comment.
Could also be RewindableRegister, emphasizing the fact that it might be rewound depending on chain state
There was a problem hiding this comment.
Went with RewindableRegister, and moved it into the contract module
| // TODO: PXE also exposes `getLogsByTagV2`, whose response additionally carries the origin block number and timestamp | ||
| // of each log. Switch this oracle (and `LogRetrievalResponse`) over to it once a consumer needs that context. | ||
| #[oracle(aztec_utl_getLogsByTag)] | ||
| #[oracle(aztec_utl_getLogsByTagV2)] |
There was a problem hiding this comment.
getLogsByTagV2 wasn't used before this PR, so we don't need to write an adapter for it
nchamo
added
the
ci-no-fail-fast
| /// # Panics | ||
| /// If `origin_block` is strictly behind the current version's origin: the current version is the one with the | ||
| /// highest origin block, not the most recently written, so a write anchored behind it would not take effect. | ||
| /// Re-writing at the current version's origin is allowed: re-recording an identical fact is a no-op, which | ||
| /// keeps rescans idempotent. |
| /// | ||
| /// The current version is the one with the highest surviving origin. Returns `None` if no version survives: | ||
| /// nothing was ever written, or a reorg retracted every version. | ||
| pub unconstrained fn read(self) -> Option<(T, RetractableFactOrigin)> |
There was a problem hiding this comment.
interesting how RetractableFactOrigin feels leaky for this, maybe we need a more general block + block state description (being nitpicky here, not for this PR)
| } | ||
|
|
||
| /// Returns every surviving version, or `None` if there is none. | ||
| unconstrained fn maybe_versions(self) -> Option<EphemeralArray<Fact>> { |
There was a problem hiding this comment.
would rename this to versions, the compiler will take care of not letting us use it without unwrapping
| assert( | ||
| origin_block.block_number >= current_version(versions).origin_block.unwrap().block_number, | ||
| "RetractableRegister write is anchored behind the current version", | ||
| ); |
There was a problem hiding this comment.
wouldn't this assertion here mean a logic error causes a crash? maybe we could make the function's contract explicitly be "writes in the past are ignored"?
There was a problem hiding this comment.
That was my original implementation, writes in the past were no-op. But I believe that a scenario like that was most likely an error in the contract logic, and I thought it made sense to raise it instead of silently ignoring it. If we know the client is doing something wrong, why hide/support it?
But I'm not against changing it back, just wanted to share my opinion. Do you believe we should go back?
There was a problem hiding this comment.
this is probably fine if we keep the module out of reach of userland, given an error in the handshake registry contract is basically a bug in Aztec.nr.
if we make this feature available to contract devs in general, what worries me is that misusing it can cause a whole tx to fail (thinking mostly of txs where there's multiple contracts interacting). maybe I'm being too conservative?
There was a problem hiding this comment.
Moved it into the registry's module
| /// Facts within a collection are insertion-ordered, so new handshakes only ever append, keeping | ||
| /// [`HandshakeRegistry::get_handshakes`] pagination stable across pages. |
There was a problem hiding this comment.
false, there's purposedly no guarantees of this, let's not rely on this fact
There was a problem hiding this comment.
I re-phrased it. It was wrong
mverzilli
left a comment
mverzilli
left a comment
There was a problem hiding this comment.
Super nice! Leaving aside the design suggestions and nitpicks, it's important to make this resilient to store insertion ordering instability. Any order reqs need to be handled explicitly.
…24511) ## Summary - Renames the HandshakeRegistry's `get_handshakes` utility to `get_non_interactive_handshakes`: it only returns handshakes discovered from non-interactive announcement logs, so interactive handshakes (#24473) never appear in it, and the name should say which kind it returns. - Sweeps the rename through the aztec-nr selector constant, the registry's internal reader, the PXE default-authorization allowlist, and tests, and documents the getter's privacy properties (any contract can read a scope's discovered handshakes; ephemeral keys don't reveal the shared secret). - Re-pins the standard contracts, since the registry's artifact changes. Stacked on #24483. Fixes F-769 --------- Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
3 participants
Why we are doing this
Discovered handshakes were persisted in capsule storage, which is not reorg-aware: a reorg left stale handshakes behind, and the sync cursor stayed past the pruned blocks, so handshakes re-mined in replacement blocks were never rescanned. Facts are tied to an origin block and PXE retracts them when that block is pruned, so storing handshakes as facts makes discovery reorg-safe, the same way F-682 did for offchain message reception.
The change
aztec_utl_getLogsByTagV2, resolving the TODO from feat(pxe): origin block number timestamp log oracle in log retrieval #24398. The response gains the log's origin block:block_numberblock_timestampblock_hash(also added to the PXE-side response, with an oracle minor version bump)get_handshakespagination stays stable.facts::RetractableRegister: a reorg-aware mutable value on top of retractable facts. Writes append versions anchored at origin blocks, reads return the newest surviving version, and rollback is performed by the store, never by the writer. History behind the newest finalized version is compacted away, and a write anchored behind the current version panics.Fixes F-766