Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: contract instance/class cache for current tx - ensure that later txs in block cannot wipe out contracts created earlier #12261

Merged
merged 12 commits into from
Mar 3, 2025

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Feb 25, 2025

Ensures that contracts only get wiped out of the cache when they really should.

What's included

  1. Separates the caches for ContractDataSourcePublicDB as follows:
    private blockCache = new TxContractCache();
    private currentTxNonRevertibleCache = new TxContractCache();
    private currentTxRevertibleCache = new TxContractCache();
    private bytecodeCommitmentCache = new Map<string, Fr>();
    
    • using a new little cache class TxContractCache.
  2. Caches non-revertibles before public setup and revertibles before public app logic.
  3. Pushes all new contracts to block-level cache for private-only txs.
  4. Includes some progress towards ensuring that contracts can be called after deployment in same block (including public-processor and e2e tests)
  5. Rearranged some things in simulator/src/public into subfolders
  6. Created a new suite of deployment tests for public processor.

Issues

I discovered during this work, that if you run certain test-cases in deploy_method.test.ts on their own (without running earlier test cases, they fail as follows (happens on master):
image

This holds true for the new test. This might mean that contracts cannot be properly called after deployment in the same block! More investigation is needed in another ticket/pr.

Also, I can run all of the deploy_method.test.ts tests locally and they pass, but in CI, the new one fails:
image

@dbanks12 dbanks12 requested a review from fcarreiro as a code owner February 28, 2025 15:39
Comment on lines -169 to 170
Math.ceil(publicContractClass.packedBytecode.length / 32) + 1,
Math.ceil(publicContractClass.packedBytecode.length / 31) + 1,
),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied this code only to realize that this 32 was wrong. Doesn't matter here because the bytecode is fake, but it mattered for real bytecode.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

LGTM! One question though: are the ContractsDataSourcePublicDB getters for classes and instances used from both public setup and public app? If so, should we disallow loading a contract class/instance created in a private revertible phase in the public setup? Otherwise, if the app logic reverts, the contract will never have been deployed, and public setup would be running on unpublished code. Does this make sense?

Copy link
Contributor

@MirandaWood MirandaWood left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Comment on lines 66 to 77
const siloedNonRevertibleContractClassLogs = tx.data.forPublic
? await tx.filterContractClassLogs(
tx.data.forPublic!.nonRevertibleAccumulatedData.contractClassLogsHashes,
/*siloed=*/ true,
)
: await tx.filterContractClassLogs(tx.data.forRollup!.end.contractClassLogsHashes, /*siloed=*/ true);
const siloedRevertibleContractClassLogs = tx.data.forPublic
? await tx.filterContractClassLogs(
tx.data.forPublic!.revertibleAccumulatedData.contractClassLogsHashes,
/*siloed=*/ true,
)
: [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good! If it's easier/cleaner I can change tx.data.getNonEmptyContractClassLogsHashes() to a pair of fns like get(Non)RevertibleContractClassLogsHashes()? IIRC I added the original to make this code clearer, and have some cc log cleanup to do anyway (#12325). LMK!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that would help clean this up! Can be done after this PR though

@dbanks12 dbanks12 requested a review from spalladino March 3, 2025 15:38
@dbanks12
Copy link
Collaborator Author

dbanks12 commented Mar 3, 2025

LGTM! One question though: are the ContractsDataSourcePublicDB getters for classes and instances used from both public setup and public app? If so, should we disallow loading a contract class/instance created in a private revertible phase in the public setup? Otherwise, if the app logic reverts, the contract will never have been deployed, and public setup would be running on unpublished code. Does this make sense?

@spalladino good catch! I updated it so that we can add non-revertible contract infos separately from revertibles. PublicTxSimulator now adds only the non-revertibles during/before setup, and then adds revertibles during/before app logic.

Also note that before this PR, none of the contract classes/instances from a private-only TX were ever added to ContractsDataSourcePublicDB since we only ever called addNewContracts in PublicTxSimulator.

@dbanks12 dbanks12 enabled auto-merge (squash) March 3, 2025 16:15
@dbanks12 dbanks12 disabled auto-merge March 3, 2025 16:17
@dbanks12 dbanks12 enabled auto-merge (squash) March 3, 2025 16:17
@dbanks12 dbanks12 merged commit 3bdb886 into master Mar 3, 2025
7 checks passed
@dbanks12 dbanks12 deleted the db/tx-cache branch March 3, 2025 20:39
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