Skip to content

feat!: constrain the GetContractInstance opcode in AVM#15430

Merged
dbanks12 merged 1 commit into
nextfrom
db/gci
Jul 15, 2025
Merged

feat!: constrain the GetContractInstance opcode in AVM#15430
dbanks12 merged 1 commit into
nextfrom
db/gci

Conversation

@dbanks12

@dbanks12 dbanks12 commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

Design-ish document

  1. Implements GetContractInstance opcode
  2. Removes timestamp from the bytecode retrieval & bytecode manager and instead include it only in update_check along with a new lookup to public inputs to constrain it.
  3. Removes deduplication of bytecode retrieval (see document)

Important question:

Is it acceptable that the GetContractInstance opcode performs two permutations to memory to accomplish its two writes? I wish I discussed this with others sooner, but I didn't think enough about it. If that's not acceptable, our other options are:

  1. Extend this gadget to use 2 rows per invocation where the second row just writes the second result value
  2. Return exists back to execution and let the execution trace handle the write from a register.

Followup work:

  1. Handle magic addresses
  2. Make sure we're actually deduplicating class id derivation, address derivation and bytecode hashing

dbanks12 commented Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

@dbanks12 dbanks12 force-pushed the db/gci branch 17 times, most recently from 400d88f to 30b0891 Compare July 8, 2025 20:41
@dbanks12 dbanks12 force-pushed the db/gci branch 4 times, most recently from c00bbff to e03eb88 Compare July 9, 2025 00:49
Comment thread barretenberg/cpp/pil/vm2/precomputed.pil Outdated
@dbanks12 dbanks12 force-pushed the db/gci branch 7 times, most recently from 2e06fea to 6f1c89d Compare July 9, 2025 20:27
@dbanks12 dbanks12 requested a review from sirasistant July 10, 2025 23:53
@dbanks12 dbanks12 force-pushed the db/gci branch 2 times, most recently from 6a07315 to ca714f0 Compare July 11, 2025 00:22
Comment thread barretenberg/cpp/pil/vm2/contract_instance_retrieval.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil
Comment thread barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil
Comment thread barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp Outdated
.dst_offset = dst_offset,
.member_enum = member_enum,
.space_id = memory.get_space_id(),
.timestamp = globals.timestamp,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the timestamp the same for the whole TX? if yes I'm surprised you need it in all the places; you might be able to avoid it in most lookups (just thinking out loud, haven't checked)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved it into only the update_check module and removed it from all of the interfaces. Update check now performs a lookup to the public inputs to retrieve timestamp. Removed it from TODOs in description!

Comment thread barretenberg/cpp/pil/vm2/contract_instance_retrieval.pil
Comment thread barretenberg/cpp/pil/vm2/bytecode/bc_retrieval.pil Outdated
Comment thread barretenberg/cpp/pil/vm2/bytecode/bc_retrieval.pil Outdated
Comment thread barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp Outdated
Comment thread barretenberg/cpp/src/barretenberg/vm2/simulation/get_contract_instance.hpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit lost now. Where was the class id / derivation checked and is it being deduplicated?

Also: you mentioned in your doc that you'd make sure we are correctly deduplicating everything that we can (class id, bytecode hashing and decomposition etc). Can you confirm this is correct?

@dbanks12 dbanks12 Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Class ID derivation is only necessary during bytecode retrieval. So it is done by the Bytecode Manager. Address derivation is done any time you need to retrieve a contract instance, so it is triggered by Contract Instance Manager. During contract instance retrieval, we just need to validate the instance, so we don't care where the class ID comes from, just that it can be used to derive the provided address.

I don't think we are de-duplicating any of those to be honest! I was not planning on addressing that in this PR. I dove deeper into this for a while today and here is where I'm at:

  1. We can deduplicate bytecode hashing & class ID derivation by contract class ID as the unique identifier. I think this is easy in simulation and circuit.
  2. We can deduplicate address derivation by address. This is also easy in simulation and circuit.
  3. Update checking, nullifier checking, contract instance retrieval and bytecode retrieval will not be deduplicated.
  4. Bytecode ID is no longer deduplicating... anything. So if things like bytecode decomposition and instruction fetching were relying on deduplication there, that won't work anymore.

I'm happy to work on this, but I'm not sure how substantial an effort it will be. So I'm tempted to say we should treat it as a separate piece of work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe it was actually easier than I thought. Take a look at this PR stacked on top. #15683

@sirasistant sirasistant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM on my side

@dbanks12 dbanks12 force-pushed the db/gci branch 5 times, most recently from 2e4e394 to 03a4646 Compare July 14, 2025 18:38
@dbanks12 dbanks12 added this pull request to the merge queue Jul 15, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 15, 2025
@dbanks12 dbanks12 added this pull request to the merge queue Jul 15, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jul 15, 2025
@dbanks12 dbanks12 force-pushed the db/gci branch 2 times, most recently from dcf75af to 0ce85f7 Compare July 15, 2025 18:05
@dbanks12 dbanks12 enabled auto-merge July 15, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants