refactor: offchain reception on entity store#24142
Conversation
mverzilli
changed the base branch from
martin/f-738-tx-resolution-oracle
to
martin/expose-entity-store-oracles
…n-reception-on-entity-store
…n-reception-on-entity-store
| set_contract_sync_cache_invalid(contract_address, messages.map(|msg| msg.recipient)); | ||
| } | ||
|
|
||
| /// Step function to advance the reception workflow of an offchain message. |
There was a problem hiding this comment.
It took me a while to understand this function, including reasoning about the correctness of the TTL. I feel like there's an explanation missing of what this entity and workflow are supposed to do, how they operate and how they handle edge cases, from which the FSM states and transitions would be derived and therefore the implementation of this function.
There was a problem hiding this comment.
In particular, I don't understand how tx-less messages are handled. The logic seems to be as follows:
A message contains a payload and timestamp, and may be related to a transaction. A message is processed only once, when we see the transaction (this is a fact). (notably tx less messages seem to never get processed)
An unprocessed message expires after the TTL. This is because a transaction known at a time T could not possibly begin to exist after the TTL, because this is larger than the maximum transaction expiration duration (which is not mentioned), therefore it is safe to stop looking for it. We add a 2 hour safety margin.
A processed message also expires after the TTL. This is because we're being a bit lazy - we could instead wait until finalization of the block associated to the tx that caused us to process it.
There was a problem hiding this comment.
The comment below explains some of these things, but I feel like the explanation already assumes you understand parts of how this works, and it also acts on a sort of 'collapsed' view of the FSM (e.g. the combination of the processed and unprocessed termination conditions, etc.).
There was a problem hiding this comment.
I don't understand how tx-less messages are handled
That's because they aren't. I re-wrote these docs to adapt them to the new implementation, but I think along the way I lost that detail. The fact that I lost the history of the file as you pointed out above certainly doesn't help there.
This is at feature parity with the previous implementation, which also didn't. I didn't want to introduce behavior changes at this point, but we can of course plan to add support in a follow up PR.
There was a problem hiding this comment.
The rest of the comments, all fair points. I like the idea of documenting the FSM more explicitly and starting from there.
| // Ask PXE to resolve each message's originating tx. We pass the tx hashes in active-reception order, so the | ||
| // resolved txs come back aligned with the reception indices and can be matched back positionally. | ||
| let resolved_txs = get_resolved_txs(active_receptions.map(|reception: Entity<OffchainMessage>| { | ||
| reception.body.tx_hash.unwrap_or(0) |
There was a problem hiding this comment.
I see now where the 0 hash in a past PR orignated from. This should be an option, with an explanation of why it makes sense to have optional queries (which is obvious here but very non-obvious in TS).
…n-reception-on-entity-store # Conflicts: # yarn-project/pxe/src/contract_function_simulator/oracle/private_execution.test.ts
…ull message Hash the whole serialized OffchainMessage (via its Serialize derive) instead of just ciphertext + tx hash, so identity covers every field and there are no silently-excluded fields to reason about.
OffchainMessage, OFFCHAIN_RECEPTION_TYPE and fact_collection_id (now a free function rather than a method) move into mod.nr; the near-empty message.nr and its round-trip test are removed.
| /// A message delivered via the `offchain_receive` utility function. | ||
| #[derive(Serialize, Deserialize)] | ||
| pub struct OffchainMessage { | ||
| /// The encrypted message payload. | ||
| pub ciphertext: BoundedVec<Field, MESSAGE_CIPHERTEXT_LEN>, | ||
| /// The intended recipient of the message. | ||
| pub recipient: AztecAddress, | ||
| /// The hash of the transaction that produced this message. `Option::none` indicates a tx-less message. | ||
| pub tx_hash: Option<Field>, | ||
| /// Anchor block timestamp at message emission. | ||
| pub anchor_block_timestamp: u64, | ||
| } | ||
|
|
||
| /// Fact-collection type id shared by every offchain message reception in the fact store. | ||
| global OFFCHAIN_RECEPTION_TYPE_ID: Field = sha256_to_field("AZTEC_NR::OFFCHAIN_RECEPTION_TYPE_ID".as_bytes()); | ||
|
|
||
| /// Computes the fact-collection id that identifies a message's reception workflow in the fact store. | ||
| /// | ||
| /// The id is a hash over the full serialized message. Re-delivering a byte-identical message therefore is idempotent, | ||
| /// and a message differing in any of its fields triggers a completely new reception workflow. | ||
| fn fact_collection_id(message: OffchainMessage) -> Field { | ||
| poseidon2_hash(message.serialize()) | ||
| } |
There was a problem hiding this comment.
Note: this is a bit of weird placement for these lines in the file. I chose to put them here so as not to scramble the previous review comments
2 participants
Re-implementation of offchain reception workflows using the new EntityStore machinery
Note this exposed a problem in the EntityStore idempotency semantics: throwing on an attempt at creating an already existing entity is problematic, because Noir doesn't provide error handling mechanisms. In the end, I decided to make entity creation idempotent, with the first attempt at creating a given entity "winning" to avoid inadvertent overwrites. It is then user responsibility to model their workflows properly (there's plenty of alternatives: change how entity id's are derived, check for existence before attempting creation, using facts, etc).
Closes F-682