Remove witness tx struct #1457

ChihChengLiang · 2023-06-05T15:15:56Z

Description

We remove the witness tx struct.

We can wait for the lo-hi refactor ready then for this PR.

Issue Link

Part of #1391

Type of change

New feature (non-breaking change which adds functionality)

@@ -320,7 +329,12 @@ impl<C: CircuitsParams> CircuitInputBuilder<C> {
        // accumulates gas across all txs in the block
        for (tx_index, tx) in eth_block.transactions.iter().enumerate() {
            let geth_trace = &geth_traces[tx_index];
-            self.handle_tx(tx, geth_trace, tx_index + 1 == eth_block.transactions.len())?;
+            self.handle_tx(


How do you think we have a var, let next_tx_index = tx_index + 1 and replace the next two tx_index + 1?

I added a new variable and a comment.
As I understand it, tx_index + 1 is not the "next" tx id. The tx_index + 1 is there because the tx id starts from 1.

KimiWu123 · 2023-07-05T04:17:47Z

zkevm-circuits/src/evm_circuit/execution/begin_tx.rs

-        let gas_fee = tx.gas_price * tx.gas;
+        let tx_id = tx.id;
+        let tx = &tx.tx;
+        let gas_fee = tx.gas_price * tx.gas_limit.as_u64();


suggest to use tx.gas() as you did in end_tx

Fixed in d33f401
I add a convenient method for gas that returns simple u64.
The reason not to change the gas_limit field from U64 to u64 is that the gas_limit field must use U64 to be appropriately serialized for JSON RPC to consume.

ChihChengLiang · 2023-07-05T06:26:49Z

zkevm-circuits/src/evm_circuit/execution/begin_tx.rs

        self.tx_callee_address
-            .assign_h160(region, offset, tx.callee_address)?;
-        self.call_callee_address.assign_h160(
-            region,
-            offset,
-            if tx.is_create {
-                get_contract_address(tx.caller_address, tx.nonce)
-            } else {
-                tx.callee_address
-            },
-        )?;
+            .assign_h160(region, offset, tx.to_or_contract_addr())?;
+        self.call_callee_address
+            .assign_h160(region, offset, tx.to_or_contract_addr())?;


Wait, I realized I confused tx_callee_address and call_callee_address.
I need to figure out what's the difference here.

I checked the deleted zkevm-circuits/src/witness/tx.rs and confirmed that tx_callee_address and call_callee_address are currently assigned with the same value.

So changes are not required here. We can optimize the constraints in future PR.

ChihChengLiang · 2023-07-05T06:29:44Z

zkevm-circuits/src/evm_circuit/execution/begin_tx.rs

-        let gas_fee = tx.gas_price * tx.gas;
+        let tx_id = tx.id;
+        let tx = &tx.tx;
+        let gas_fee = tx.gas_price * tx.gas_limit.as_u64();


Fixed in d33f401
I add a convenient method for gas that returns simple u64.
The reason not to change the gas_limit field from U64 to u64 is that the gas_limit field must use U64 to be appropriately serialized for JSON RPC to consume.

ChihChengLiang · 2023-07-05T06:31:18Z

bus-mapping/src/circuit_input_builder.rs

@@ -320,7 +329,12 @@ impl<C: CircuitsParams> CircuitInputBuilder<C> {
        // accumulates gas across all txs in the block
        for (tx_index, tx) in eth_block.transactions.iter().enumerate() {
            let geth_trace = &geth_traces[tx_index];
-            self.handle_tx(tx, geth_trace, tx_index + 1 == eth_block.transactions.len())?;
+            self.handle_tx(


I added a new variable and a comment.
As I understand it, tx_index + 1 is not the "next" tx id. The tx_index + 1 is there because the tx id starts from 1.

hero78119

Nice job! Fantastic to remove one more technical burden <3

I would like to propose a minor change to make coding style looks more comfortable.

impl std::ops::Deref for Transaction {
    type Target = geth_types::Transaction;

    fn deref(&self) -> &Self::Target {
        &self.tx
    }
}

By dereference, It will make all access to inner tx like itself, so we save save one nested access!

So

tx.tx.xxxx become tx.xxx
tx.tx.clone() become *tx.clone()

ChihChengLiang · 2023-07-05T14:37:34Z

@hero78119 Deref is beautiful! Thanks for the suggestion.

hero78119

LGTM! 🎆 :) thanks for the quick fix

ChihChengLiang · 2023-07-05T15:47:41Z

bus-mapping/src/circuit_input_builder/transaction.rs

-    pub fn is_create(&self) -> bool {
-        self.calls[0].is_create()
-    }


This method is causing the test to fail.

For padding tx in tx circuit, the calls vector is empty. So that querying is_create would panic.
We use the is_create property in witness::Transaction before this PR, which relies on the is_create method of geth_types::Transaction shown below. The below method is more robust because it doesn't panic.

We now remove the circuit_input_builder::Transaction's is_create. Any tx.is_create() call will propagate to geth_types::Transaction's is_create, due to the deref effect. It would be the same method at work before this PR.

pub fn is_create(&self) -> bool { self.to.is_none() }

ed255 linked an issue Jun 15, 2023 that may be closed by this pull request

Merge structs between CIB and evm circuit witness #1391

Closed

5 tasks

ed255 mentioned this pull request Jun 23, 2023

Merge structs between CIB and evm circuit witness #1391

Closed

5 tasks

ChihChengLiang force-pushed the rm-witness-tx-2 branch from 77305d1 to d3784a2 Compare June 30, 2023 10:12

ChihChengLiang marked this pull request as draft June 30, 2023 10:12

ChihChengLiang force-pushed the rm-witness-tx-2 branch 2 times, most recently from 354503c to 3e53d37 Compare July 4, 2023 14:14

rm witness tx

c545e14

ChihChengLiang force-pushed the rm-witness-tx-2 branch from 3e53d37 to c545e14 Compare July 4, 2023 14:23

eth_block back

bbb3067

ChihChengLiang force-pushed the rm-witness-tx-2 branch from 7dae6a7 to bbb3067 Compare July 4, 2023 15:30

simplify assignment

e0a6e13

ChihChengLiang marked this pull request as ready for review July 4, 2023 15:48

KimiWu123 self-requested a review July 5, 2023 02:52

KimiWu123 approved these changes Jul 5, 2023

View reviewed changes

ChihChengLiang added 2 commits July 5, 2023 14:19

simplify the gas expression

d33f401

comment on tx id

e85375c

github-actions bot added the crate-eth-types Issues related to the eth-types workspace member label Jul 5, 2023

ChihChengLiang commented Jul 5, 2023

View reviewed changes

make gas limit public so that testool won't panic

ac1c901

hero78119 self-requested a review July 5, 2023 10:16

hero78119 reviewed Jul 5, 2023

View reviewed changes

get rid of awkward tx.tx with Deref trait

43a4258

hero78119 approved these changes Jul 5, 2023

View reviewed changes

fix test by using the more robust method

9e56086

ChihChengLiang commented Jul 5, 2023

View reviewed changes

ChihChengLiang added this pull request to the merge queue Jul 5, 2023

Merged via the queue into main with commit dd1e1c4 Jul 5, 2023

ChihChengLiang deleted the rm-witness-tx-2 branch July 5, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove witness tx struct #1457

Remove witness tx struct #1457

ChihChengLiang commented Jun 5, 2023 •

edited

Loading

KimiWu123 left a comment

KimiWu123 Jul 5, 2023

ChihChengLiang Jul 5, 2023

KimiWu123 Jul 5, 2023

ChihChengLiang Jul 5, 2023

ChihChengLiang Jul 5, 2023

ChihChengLiang Jul 5, 2023 •

edited

Loading

ChihChengLiang Jul 5, 2023

ChihChengLiang Jul 5, 2023

hero78119 left a comment •

edited

Loading

ChihChengLiang commented Jul 5, 2023

hero78119 left a comment

ChihChengLiang Jul 5, 2023

Remove witness tx struct #1457

Remove witness tx struct #1457

Conversation

ChihChengLiang commented Jun 5, 2023 • edited Loading

Description

Issue Link

Type of change

Contents

KimiWu123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChihChengLiang Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hero78119 left a comment • edited Loading

Choose a reason for hiding this comment

ChihChengLiang commented Jul 5, 2023

hero78119 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChihChengLiang commented Jun 5, 2023 •

edited

Loading

ChihChengLiang Jul 5, 2023 •

edited

Loading

hero78119 left a comment •

edited

Loading