feat: initial siloing of incoming notes to pxe / pxe database #7533
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sklppy88
changed the title
sklppy88
changed the title
sklppy88
force-pushed
the
ek/feat/silo-notes-to-account-in-pxe
branch
8 times, most recently
from
68388da
to
b8ed79f
Compare
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
sklppy88
force-pushed
the
ek/feat/silo-notes-to-account-in-pxe
branch
from
b8ed79f
to
95464a1
Compare
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
sklppy88
force-pushed
the
ek/feat/silo-notes-to-account-in-pxe
branch
3 times, most recently
from
918203b
to
7d1deb0
Compare
| addNote(note: ExtendedNote): Promise<void> { | ||
| return this.pxe.addNote(note); | ||
| addNote(note: ExtendedNote, account: AztecAddress): Promise<void> { | ||
| return this.pxe.addNote(note, account); |
There was a problem hiding this comment.
This is weird and a prime example on why we should expose PXE directly: add a note to this siloed storage shouldn't be the wallet's responsability
There was a problem hiding this comment.
ExtendedNote does have an owner, which seems to be related to the incoming keys used. So we could go with that initially. If the note had an npk, then the owner of the npk sounds perhaps more conceptually correct (assuming they were not the same). And if there's no npk and no encryption, then who knows.
There was a problem hiding this comment.
This entire flow seems quite incomplete due to the partial implementation of keys, especially key rotation. So this would definitely need reviewing later on. But it does sound in principle like the notion of notes being 'owned' makes sense.
There was a problem hiding this comment.
Thank you for your work on this PR! I see a lot of effort went into it and the multiple e2e tests 😅
As we discussed, I'm concerned that introducing this change in this manner might result in excessive API pollution, affecting many parts of PXE with the extra account parameter. The large diff shows a significant impact on numerous callsites.
Given we're still exploring the best implementation, it might be challenging to roll back these changes if needed. I was thinking we could start with something more contained within PXE to minimize the impact on callsites. Since the callers are quite basic (raw test calls, barebones wallet, etc.), they might not handle this additional complexity well, leading to more plumbing to their callers.
At this early experimental stage, I'd suggest we explore different siloing methods (extending keys, using distinct databases, applying filters on top of the data layer, etc.). We could assess the tradeoffs (performance, attack surface size, multi-account access) and cosnider a path forward that:
a) avoids making the e2e setup overly complicated, using escape hatches if needed
b) acknowledges it will take time to develop a proper wallet using the full-fledged feature, and so allows for intermediate milestones
c) allows for some back-and-forth as we refine our decisions
| @@ -73,7 +73,8 @@ export class BatchCall extends BaseContractInteraction { | |||
|
|
|||
| const unconstrainedCalls = unconstrained.map(async indexedCall => { | |||
| const call = indexedCall[0]; | |||
| return [await this.wallet.simulateUnconstrained(call.name, call.args, call.to, options?.from), indexedCall[1]]; | |||
| // This is weird | |||
There was a problem hiding this comment.
It'd be useful if these kind of comments were expanded some more, e.g. what it is that's weird, how it might be desirable to do it, why it might not be possible, etc.
| addNote(note: ExtendedNote): Promise<void> { | ||
| return this.pxe.addNote(note); | ||
| addNote(note: ExtendedNote, account: AztecAddress): Promise<void> { | ||
| return this.pxe.addNote(note, account); |
There was a problem hiding this comment.
ExtendedNote does have an owner, which seems to be related to the incoming keys used. So we could go with that initially. If the note had an npk, then the owner of the npk sounds perhaps more conceptually correct (assuming they were not the same). And if there's no npk and no encryption, then who knows.
| @@ -57,7 +57,7 @@ async function main() { | |||
| TokenContract.notes.TransparentNote.id, | |||
| receipt.txHash, | |||
| ); | |||
| await pxe.addNote(extendedNote); | |||
| await pxe.addNote(extendedNote, alice.address); | |||
There was a problem hiding this comment.
Related to the above, see how here alice was the owner of the note being stored.
| addNote(note: ExtendedNote): Promise<void> { | ||
| return this.pxe.addNote(note); | ||
| addNote(note: ExtendedNote, account: AztecAddress): Promise<void> { | ||
| return this.pxe.addNote(note, account); |
There was a problem hiding this comment.
This entire flow seems quite incomplete due to the partial implementation of keys, especially key rotation. So this would definitely need reviewing later on. But it does sound in principle like the notion of notes being 'owned' makes sense.
| getIncomingNotes(filter: IncomingNotesFilter, account: AztecAddress): Promise<IncomingNoteDao[]> { | ||
| console.log('GETTING INCOMING NOTES FROM KV PXE DATABASE WITH ACCOUNT', account); | ||
| console.log(filter); | ||
|
|
There was a problem hiding this comment.
I think this is the source of most of my confusion. filter already has an owner element - could we not simply use that?
| @@ -246,15 +260,17 @@ export class KVPxeDatabase implements PxeDatabase { | |||
|
|
|||
| candidateNoteSources.push({ | |||
| ids: publicKey | |||
| ? this.#notesByIvpkM.getValues(publicKey.toString()) | |||
| ? this.#notesByIvpkM.getValues(`${account.toString()}:${publicKey.toString()}`) | |||
There was a problem hiding this comment.
Doesn't doing things this way mean that we always access one account at a time? So if e.g. multiple accounts were unlocked for whatever reason, we'd need to loop over all of them every time we fetch anything, which seems to lead to a lot of duplicated effort. Is there no way we can avoid this?
| @@ -227,7 +227,7 @@ export class NoteProcessor { | |||
| const incomingNotes = blocksAndNotes.flatMap(b => b.incomingNotes); | |||
| const outgoingNotes = blocksAndNotes.flatMap(b => b.outgoingNotes); | |||
| if (incomingNotes.length || outgoingNotes.length) { | |||
| await this.db.addNotes(incomingNotes, outgoingNotes); | |||
| await this.db.addNotes(incomingNotes, outgoingNotes, this.account); | |||
There was a problem hiding this comment.
Ah this is interesting, I thought the note processor would be handling multiple sets of keys, but it looks like each account gets their own note processor. This seems different from other pieces of the stack, where we are handling many accounts at once (like when fetching notes, they're all in the same database).
sklppy88
force-pushed
the
ek/feat/silo-notes-to-account-in-pxe
branch
from
7d1deb0
to
dda9474
Compare
sklppy88
force-pushed
the
ek/feat/silo-notes-to-account-in-pxe
branch
from
dda9474
to
8b0d182
Compare
Please read contributing guidelines and remove this line.