Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Feat/CREATE Part C - bus-mapping and gadget implementation #1430

Merged
merged 19 commits into from
Jun 9, 2023

Conversation

KimiWu123
Copy link
Contributor

Description

NOTE: This is an updated version of #1358

This PR is actually based on top of #1425

Issue Link

#1130

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

Bus mapping implementation for CREATE/CREATE2
EVM circuit's gadget for CREATE/CREATE2

How Has This Been Tested?

Tests can be found here: https://github.com/scroll-tech/zkevm-circuits/blob/2d2bfc6ccf179ade1a8d063f9586b93e5283a557/zkevm-circuits/src/evm_circuit/execution/create.rs#L678

@KimiWu123 KimiWu123 self-assigned this May 23, 2023
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-mock Issues related to the mock workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 23, 2023
@KimiWu123 KimiWu123 changed the title [WIP] Feat/create part c - CREATE/CREATE2 implementation [WIP] Feat/CREATE Part C - CREATE/CREATE2 implementation May 23, 2023
@KimiWu123 KimiWu123 changed the title [WIP] Feat/CREATE Part C - CREATE/CREATE2 implementation [WIP] Feat/CREATE Part C - bus-mapping and gadget implementation May 23, 2023
@github-actions github-actions bot removed the crate-mock Issues related to the mock workspace member label May 26, 2023
@ed255 ed255 linked an issue May 31, 2023 that may be closed by this pull request
@KimiWu123 KimiWu123 marked this pull request as ready for review June 5, 2023 03:05
@KimiWu123 KimiWu123 changed the title [WIP] Feat/CREATE Part C - bus-mapping and gadget implementation Feat/CREATE Part C - bus-mapping and gadget implementation Jun 5, 2023
@KimiWu123 KimiWu123 requested a review from silathdiir June 5, 2023 09:44
Copy link
Contributor

@silathdiir silathdiir left a comment

Choose a reason for hiding this comment

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

Add small comments, others LGTM.

Hi @KimiWu123, please help resolve the fixed conversations. Seems there is no Resolve conversation button on my side. Thanks.

bus-mapping/src/evm/opcodes/callop.rs Outdated Show resolved Hide resolved
@miha-stopar miha-stopar self-requested a review June 5, 2023 12:03
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

Great work! Added just a couple of questions.

CircuitTestBuilder::new_from_test_ctx(ctx).run();
}

// RETURN or REVERT with data of [0x60; 5]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a short description why this particular initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The init code content doesn't matter, could put any random data. We don't really run the piece of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I thought so - perhaps just add a comment that any code would do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in ec9c6de

code
}

fn creater_bytecode(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: perhaps change creater to creator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 59cf61f

code.append(&bytecode! {
PUSH1(initialization_bytes.len()) // size
PUSH1(32 - initialization_bytes.len()) // length
PUSH2(23414) // value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
PUSH2(23414) // value
PUSH2(23414) // value (endowment in CREATE2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 59cf61f

let gas_cost = GasCost::CREATE.expr() + memory_expansion.gas_cost() + keccak_gas_cost;
let gas_remaining = cb.curr.state.gas_left.expr() - gas_cost.clone();
let gas_left = ConstantDivisionGadget::construct(cb, gas_remaining.clone(), 64);
let callee_gas_left = gas_remaining - gas_left.quotient();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does there need to be also the reduction of gas (for CreateDataGas) for storing the code when there is no error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! fixed in cccd6a8

@KimiWu123 KimiWu123 enabled auto-merge June 9, 2023 00:49
@KimiWu123 KimiWu123 added this pull request to the merge queue Jun 9, 2023
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I approve for @silathdiir to make the auto-merge work.

Merged via the queue into main with commit 9ab662b Jun 9, 2023
@KimiWu123 KimiWu123 deleted the feat/create-part-c branch June 9, 2023 13:26
@lispc lispc restored the feat/create-part-c branch October 3, 2023 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement CREATE & CREATE2
5 participants