Skip to content

Commit

Permalink
feat: nullify just-added notes (#12552)
Browse files Browse the repository at this point in the history 
Back when PXE was a service processing all blocks, we'd remove nullified
notes as we saw their nullifiers. This later got changed, and as of
#10722 we remove all
nullified notes whenever we 'sync' notes. However, note syncing is
becoming less and less a part of PXE, and as of
#12391 we even
deliver notes _outside_ of the PXE-led note syncing process (whenever we
complete partial note). This causes problems because we end up adding
notes, failing to realize they've been nullified and then returning them
via `get_notes` (which is what causes some tests in
#12391 to fail). The
next time a contract function is run we'll do note syncing again and
they'll be then removed, but we did have a full fn call in which they
were available.

This PR makes it so we always check if newly-added notes have been
nullified, and remove them if so. I also added some explanations re. why
we're doing things this way, created some follow-up issues (mostly
#12550 and
#12553), and
inlined `produceNoteDaos` to have the whole thing happen in a single
place. I think it's now more readable but potentially slightly large -
perhaps this will improve as we split `PxeOracleInterface` in multiple
files or modules.
  • Loading branch information
@nventuro
nventuro authored Mar 7, 2025
1 parent e7b5353 commit dcba7a4
Showing 1 changed file with 79 additions and 60 deletions.
139 changes: 79 additions & 60 deletions yarn-project/pxe/src/pxe_oracle_interface/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,6 @@ export class PXEOracleInterface implements ExecutionDataProvider {
return;
}

// Called when notes are delivered, usually as a result to a call to the process_log contract function
public async deliverNote(
contractAddress: AztecAddress,
storageSlot: Fr,
Expand All @@ -701,23 +700,96 @@ export class PXEOracleInterface implements ExecutionDataProvider {
txHash: Fr,
recipient: AztecAddress,
): Promise<void> {
const noteDao = await this.produceNoteDao(
// We are going to store the new note in the NoteDataProvider, which will let us later return it via `getNotes`.
// There's two things we need to check before we do this however:
// - we must make sure the note does actually exist in the note hash tree
// - we need to check if the note has already been nullified
//
// Failing to do either of the above would result in circuits getting either non-existent notes and failing to
// produce inclusion proofs for them, or getting nullified notes and producing duplicate nullifiers, both of which
// are catastrophic failure modes.
//
// Note that adding a note and removing it is *not* equivalent to never adding it in the first place. A nullifier
// emitted in a block that comes after note creation might result in the note being de-nullified by a chain reorg,
// so we must store both the note hash and nullifier block information.

// We avoid making node queries at 'latest' since we don't want to process notes or nullifiers that only exist ahead
// in time of the locally synced state.
// Note that while this technically results in historical queries, we perform it at the latest locally synced block
// number which *should* be recent enough to be available, even for non-archive nodes.
const syncedBlockNumber = (await this.syncDataProvider.getBlockNumber())!;
// TODO (#12559): handle undefined syncedBlockNumber
// if (syncedBlockNumber === undefined) {
// throw new Error(`Attempted to deliver a note with an unsynchronized PXE - this should never happen`);
//}

// By computing siloed and unique note hashes ourselves we prevent contracts from interfering with the note storage
// of other contracts, which would constitute a security breach.
const uniqueNoteHash = await computeUniqueNoteHash(nonce, await siloNoteHash(contractAddress, noteHash));
const siloedNullifier = await siloNullifier(contractAddress, nullifier);

// We store notes by their index in the global note hash tree, which has the convenient side effect of validating
// note existence in said tree.
const [uniqueNoteHashTreeIndex] = await this.aztecNode.findLeavesIndexes(
syncedBlockNumber,
MerkleTreeId.NOTE_HASH_TREE,
[uniqueNoteHash],
);
if (uniqueNoteHashTreeIndex === undefined) {
throw new Error(
`Note hash ${noteHash} (uniqued as ${uniqueNoteHash}) is not present on the tree at block ${syncedBlockNumber} (from tx ${txHash})`,
);
}

// TODO (#12550): findLeavesIndexes should probably return an InBlock, same as findNullifiersIndexesWithBlock. This
// would save us from having to then query the tx receipt in order to get the block hash and number. Note regardless
// that we're not validating that the note was indeed created in this tx (which would require inspecting the tx
// effects), so maybe what we really want is an InTx.
const txReceipt = await this.aztecNode.getTxReceipt(new TxHash(txHash));
if (txReceipt === undefined) {
throw new Error(`Failed to fetch tx receipt for tx hash ${txHash} when searching for note hashes`);
}

// TODO(#12549): does it make sense to store the recipient's address point instead of just its address?
const recipientAddressPoint = await recipient.toAddressPoint();
const noteDao = new NoteDao(
new Note(content),
contractAddress,
storageSlot,
nonce,
content,
noteHash,
nullifier,
txHash,
recipient,
siloedNullifier,
new TxHash(txHash),
txReceipt.blockNumber!,
txReceipt.blockHash!.toString(),
uniqueNoteHashTreeIndex,
recipientAddressPoint,
NoteSelector.empty(), // TODO(#12013): remove
);

await this.noteDataProvider.addNotes([noteDao], recipient);
this.log.verbose('Added note', {
contract: noteDao.contractAddress,
slot: noteDao.storageSlot,
nullifier: noteDao.siloedNullifier.toString,
noteHash: noteDao.noteHash,
nullifier: noteDao.siloedNullifier.toString(),
});

const [nullifierIndex] = await this.aztecNode.findNullifiersIndexesWithBlock(syncedBlockNumber, [siloedNullifier]);
if (nullifierIndex !== undefined) {
const { data: _, ...blockHashAndNum } = nullifierIndex;
await this.noteDataProvider.removeNullifiedNotes(
[{ data: siloedNullifier, ...blockHashAndNum }],
recipientAddressPoint,
);

this.log.verbose(`Removed just-added note`, {
contract: contractAddress,
slot: storageSlot,
noteHash: noteHash,
nullifier: siloedNullifier.toString(),
});
}
}

public async getLogByTag(tag: Fr): Promise<LogWithTxData | null> {
Expand Down Expand Up @@ -787,59 +859,6 @@ export class PXEOracleInterface implements ExecutionDataProvider {
}
}

async produceNoteDao(
contractAddress: AztecAddress,
storageSlot: Fr,
nonce: Fr,
content: Fr[],
noteHash: Fr,
nullifier: Fr,
txHash: Fr,
recipient: AztecAddress,
): Promise<NoteDao> {
// We need to validate that the note does indeed exist in the world state to avoid adding notes that are then
// impossible to prove.

const receipt = await this.aztecNode.getTxReceipt(new TxHash(txHash));
if (receipt === undefined) {
throw new Error(`Failed to fetch tx receipt for tx hash ${txHash} when searching for note hashes`);
}

// Siloed and unique hashes are computed by us instead of relying on values sent by the contract to make sure
// we're not e.g. storing notes that belong to some other contract, which would constitute a security breach.
const uniqueNoteHash = await computeUniqueNoteHash(nonce, await siloNoteHash(contractAddress, noteHash));
const siloedNullifier = await siloNullifier(contractAddress, nullifier);

// We store notes by their index in the global note hash tree, which has the convenient side effect of validating
// note existence in said tree. Note that while this is technically a historical query, we perform it at the latest
// locally synced block number which *should* be recent enough to be available. We avoid querying at 'latest' since
// we want to avoid accidentally processing notes that only exist ahead in time of the locally synced state.
const syncedBlockNumber = await this.syncDataProvider.getBlockNumber();
const uniqueNoteHashTreeIndex = (
await this.aztecNode.findLeavesIndexes(syncedBlockNumber!, MerkleTreeId.NOTE_HASH_TREE, [uniqueNoteHash])
)[0];
if (uniqueNoteHashTreeIndex === undefined) {
throw new Error(
`Note hash ${noteHash} (uniqued as ${uniqueNoteHash}) is not present on the tree at block ${syncedBlockNumber} (from tx ${txHash})`,
);
}

return new NoteDao(
new Note(content),
contractAddress,
storageSlot,
nonce,
noteHash,
siloedNullifier,
new TxHash(txHash),
receipt.blockNumber!,
receipt.blockHash!.toString(),
uniqueNoteHashTreeIndex,
await recipient.toAddressPoint(),
NoteSelector.empty(), // TODO(#12013): remove
);
}

async callProcessLog(
contractAddress: AztecAddress,
logPlaintext: Fr[],
Expand Down

1 comment on commit dcba7a4

@AztecBot
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: dcba7a4 Previous: 7e89dfb Ratio
wasmconstruct_proof_ultrahonk_power_of_2/20 11676.987714 ms/iter 10112.797818 ms/iter 1.15

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Please sign in to comment.