feat!: CLAUDE-GENERATED use class ID to deduplicate bytecode hashing and class ID derivation, use address for address derivation#15683
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dbanks12
requested review from
IlyasRidhuan,
Maddiaa0,
fcarreiro and
jeanmon
as code owners
dbanks12
force-pushed
the
db/dedup
branch
2 times, most recently
from
0bc88e8 to
b641799
Compare
fcarreiro
left a comment
fcarreiro
left a comment
There was a problem hiding this comment.
BTW, this is why I was asking if deduplication was done in your PR, I thought you would: https://docs.google.com/document/d/1CNHZrtBukR4CMRxF2vKnYiC77C-6J6ChOrqO9Gu_xr8/edit?disco=AAABnM22Gmw
| void ClassIdDerivation::assert_derivation(const ContractClassId& class_id, const ContractClass& klass) | ||
| { | ||
| // TODO: Cache and deduplicate. | ||
| if (derived_class_ids.contains(class_id)) { |
There was a problem hiding this comment.
I think here you could just make the event emitter a deduplicated event emitter and avoid all this. In non-assert mode, they will be equivalent.
There was a problem hiding this comment.
Yikes yeah I didn't actually insert anywhere
| FF BytecodeHasher::compute_public_bytecode_commitment([[maybe_unused]] const BytecodeId bytecode_id, | ||
| const std::vector<uint8_t>& bytecode) | ||
| { | ||
| if (bytecode_commitments.contains(bytecode_id)) { |
There was a problem hiding this comment.
Same, when do you add to the map?
|
|
||
| void AddressDerivation::assert_derivation(const AztecAddress& address, const ContractInstance& instance) | ||
| { | ||
| if (derived_addresses.contains(address)) { |
There was a problem hiding this comment.
This one has a more expensive computation, so you could chose to add a set (but you still need to insert at some point!). However, it might be simpler to just do it with a deduplicating event emitter.
| } | ||
|
|
||
| ContractClassId current_class_id = maybe_instance.value().current_class_id; | ||
| BytecodeId bytecode_id = BytecodeId(current_class_id); |
There was a problem hiding this comment.
I'd have to sit down quite some time to convince myself that using bytecode id works... in PIL. A few concerns
(1) in any given table that has bytecodeid how would you constrain (if needed) that there are no duplicates. Observe that even if two identical rows are not bad, you could claim 2 different commitments for the same bytecode id and that would be bad. With bytecode ids (and not class ids) you can ask for monotonically increasing numbers. Can we/should we do it for class id and how?
(2) It feels bytecode ids and class ids are being mixed. If you think that class id can be used, then maybe just keep/rename things to class id where it is what it represents.
I'd like to be sure that you think things will work before I spend a few hours myself looking in detail at all the connections. LMK!
There was a problem hiding this comment.
I hadn't really thought 1 all the way through. I guess this was handled for bc_retrieval.pil just by enforcing that bytecode_id is doing ++ on each row.... Which I didn't realize and didn't unconstrain in this PR 😅
There was a problem hiding this comment.
- Yes, bytecode and class IDs are being mixed. Or well.... they're
==except when it doesn't exist. I agree that maybe it makes sense to just use "class ID".
There was a problem hiding this comment.
Observe that even if two identical rows are not bad, you could claim 2 different commitments for the same bytecode id and that would be bad.
I'm not convinced that claiming 2 different commitments for a bytecode ID would actually be bad if the bytecode ID == class ID. Because you'd have a constraint failure when one of your 2 different commitments doesn't match the commitment used when deriving the class ID. So I don't think we actually need to constrain that bytecode ID is deduplicated.
Although as discussed last week, we do need to constrain that bytecode ID is right
There was a problem hiding this comment.
I'll write up a document with my thoughts and send it your way.
dbanks12
force-pushed
the
db/gci
branch
2 times, most recently
from
03a4646 to
96f94f2
Compare
dbanks12
force-pushed
the
db/gci
branch
2 times, most recently
from
0ce85f7 to
d6eb37a
Compare
dbanks12
force-pushed
the
db/emitnullifier
branch
from
8427cb6 to
c920de5
Compare
dbanks12
force-pushed
the
db/dedup
branch
2 times, most recently
from
2f9bbfa to
8689ea0
Compare
dbanks12
force-pushed
the
graphite-base/15683
branch
from
f52457d to
5f4f579
Compare
dbanks12
changed the title
dbanks12
changed the title
|
Replaced with #15977 |
2 participants
Changes bytecode ID to just be the same as class ID, unless bytecode retrieval fails in which case execution just uses bytecode ID of 0.
Adds deduplication to address derivation, class ID derivation and bytecode hashing.