feat: BeginTx and EndTx support taiko's 1559 #18

johntaiko · 2023-07-31T10:55:32Z

Brechtpd

Quickly looked at it, overall looks good.

Because some balance updates are now done conditionally, you will probably have some issues with the rw counter. So correctly updating the rw counter will be necessary in the state transition, or alternatively you could keep all the balance update but just use 0 as the delta, but I remember the code being smart enough to then skip the update so the rw counter may still not need incrementing.

If you have problems, let me know because for the invalid transactions similar code for this was necessary: taikoxyz#115

bus-mapping/src/evm/opcodes/begin_end_tx.rs

Brechtpd

Looks good! Added some questions/minor suggestions.

I do have some questions about taiko_test_util and taiko_mock. I remember on the call you said there were troubles otherwise, but is full duplication of this code required to fix the problem or will it be possible with some extra effort to just modify the code instead? I haven't looked into that code in detail so I'm not sure what the problem is. Could you explain a bit more?

I also haven't run the test yet because make test-all does not run without changes, I think just some small issues that I haven't tried fixing myself yet: taiko_mock is marked as optional (not sure why) and there's still an error in the taiko super circuit benchmark.

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

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

Brechtpd · 2023-08-08T20:15:53Z

bus-mapping/src/circuit_input_builder/input_state_ref.rs

+
+            // panic!(
+            //     "RWTable Account field {:?} lookup to non-existing account rwc: {}, op: {:?}",
+            //     rw, self.block_ctx.rwc.0, op
+            // );


Just marking it so it's not forgotten to be uncommented

Now, we add zero value to an empty coinbase with anchor transaction. So maybe I need to add a flag to distinguish between anchor and others

I believe this is still something that need to be fixed in general, also on the main PSE branch, so okay for now.

Brechtpd · 2023-08-08T22:35:54Z

zkevm-circuits/src/evm_circuit/param.rs

-pub const N_BYTE_LOOKUPS: usize = 24;
+pub const N_BYTE_LOOKUPS: usize = 28;


Instead maybe MAX_STEP_HEIGHT can be increased so other opcodes don't get more expensive, but not important for now.

Where this 4 more lookups come from?

More words are allocated in the end_tx step, so more data needs to be stored in these steps. So either the circuit can be made wider with more capacity for byte values (what this change does) or we just allow the step to use more rows (currently limited by MAX_STEP_HEIGHT) to store all the necessary data.

So, as we prefer height over width, giving more space to MAX_STEP_HEIGHT makes more sense, is it?

Generally yes. And in this specific case especially true because increasing the max height just allows the end_tx to use more rows, which doesn't impact the cost of the other opcodes. Making the circuit wider or increasing the number of lookups also impacts the cost of the other opcodes because that cost is for each row, regardless of what opcode is being processed.

Where this 4 more lookups come from?

I test the number one by one, because I need more cells in like: Word<F>(RandomLinearCombination)

tx_gas_tip_cap, tx_gas_fee_cap, base_fee_plus_tip = 3* 32 cells

mul_base_fee_by_gas_used = 32 + 8 cells

treasury updatebalance = 32 * 2 cells

and two AddWords = 2 cells

total ≈ 200+

our MAX_STEP_HEIGHT equals to 21, so we need more 10 columns, but N_BYTE_LOOKUPS has some remaining
so I test to increase columns one by one

Yeah, I think it makes sense to increase the MAX_STEP_HEIGHT

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

Brechtpd · 2023-08-09T00:34:32Z

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

+        let mul_base_fee_by_gas_used =
+            MulWordByU64Gadget::construct(cb, base_fee.clone(), gas_used.clone());
+
+        // check gas_price == min(base_fee + gas_tip_cap, gas_fee_cap)


Okay I think this is correct but it took me a while to figure that out! Because on https://eips.ethereum.org/EIPS/eip-1559: we have the following:

# priority fee is capped because the base fee is filled first priority_fee_per_gas = min(transaction.max_priority_fee_per_gas, transaction.max_fee_per_gas - block.base_fee_per_gas) # signer pays both the priority fee and the base fee effective_gas_price = priority_fee_per_gas + block.base_fee_per_gas

So this is a bit different than what is written here, but if the base_fee is subtracted from the formula than we do get to the same formula as in the EIP:

tx_gas_price = effective_tip + base_fee -> effective_tip = tx_gas_price - base_fee tx_gas_price == min(tx.gas_tip_cap + base_fee, tx.gas_fee_cap) -> (subtract base_fee everywhere) tx_gas_price - base_fee == min(tx.gas_tip_cap, tx.gas_fee_cap - base_fee) -> effective_tip = min(tx.gas_tip_cap, tx.gas_fee_cap - base_fee)

So checks out! But I'm wondering why you didn't simply use the equation from the EIP here? tx_gas_price and effective_tip are already linked in sub_gas_price_by_base_fee so the original equation should also be okay I think.

Yeah, I found both of these two approaches are used in taiko-geth:

https://github.com/taikoxyz/taiko-geth/blob/73c5b3e7d77a02a1d0e4afb2b29210bbcc9ffb4f/accounts/abi/bind/backends/simulated.go#L636

https://github.com/taikoxyz/taiko-geth/blob/73c5b3e7d77a02a1d0e4afb2b29210bbcc9ffb4f/core/state_transition.go#L396

So, maybe it's good to align with eip

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

external-tracer/src/lib.rs

taiko-mock/src/anchor.rs

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

smtmfft · 2023-08-09T02:54:06Z

zkevm-circuits/src/evm_circuit/param.rs

-pub const N_BYTE_LOOKUPS: usize = 24;
+pub const N_BYTE_LOOKUPS: usize = 28;


Where this 4 more lookups come from?

Co-authored-by: Brecht Devos <[email protected]>

smtmfft · 2023-08-10T03:09:09Z

LGTM! I think the shanghai upgrade alignment can be a new PR for new gas config and PUSH0, which could be after test with real testnet data.

Co-authored-by: Brecht Devos <[email protected]>

johntaiko · 2023-08-10T07:18:13Z

@smtmfft @Brechtpd All comments are fixed, please review again

johntaiko · 2023-08-10T08:10:54Z

LGTM! I think the shanghai upgrade alignment can be a new PR for new gas config and PUSH0, which could be after test with real testnet data.

Add TODOs

johntaiko · 2023-08-10T13:54:33Z

bus-mapping/src/circuit_input_builder/input_state_ref.rs

    ) -> Result<(), Error> {
        let (found, sender_account) = self.sdb.get_account(&sender);
        if !found {
            return Err(Error::AccountNotFound(sender));
        }
        let mut sender_balance_prev = sender_account.balance;
-        if !is_anchor_tx {


We use zero value fee instead of a flag

Brechtpd · 2023-08-11T00:22:13Z

Changes look good!

I do have some questions about taiko_test_util and taiko_mock. I remember on the call you said there were troubles otherwise, but is full duplication of this code required to fix the problem or will it be possible with some extra effort to just modify the code instead? I haven't looked into that code in detail so I'm not sure what the problem is. Could you explain a bit more?

I also haven't run the test yet because make test-all does not run without changes.

Would still like to know some more about the points above

I also don't really know how to run the tests now because it seems taiko_mock is required to do that, but I only see it used in begin_tx and most of the tests fail there for me as well so I think I'm doing something wrong.

johntaiko · 2023-08-11T05:37:04Z

Would still like to know some more about the points above

I also don't really know how to run the tests now because it seems taiko_mock is required to do that, but I only see it used in begin_tx and most of the tests fail there for me as well so I think I'm doing something wrong.

Yeah, lots of test cases broke after added anchor transaction. And I think it's low priority to fix these tests.
I will commit another PR for this case after the aggregation integrating

I also commit a new PR in our Taiko's repository, because the rlp-circuit will not be added in next testnet. Please see taikoxyz#136

johntaiko added 2 commits July 31, 2023 10:40

feat: BeginTx and EndTx support taiko's 1559

4da9a36

chore: update gitignore

a7d04c3

johntaiko requested a review from smtmfft July 31, 2023 10:55

johntaiko linked an issue Jul 31, 2023 that may be closed by this pull request

zkevm circuit 1559 support #16

Open

8 tasks

github-actions bot added crate-bus-mapping crate-integration-tests crate-zkevm-circuits labels Jul 31, 2023

johntaiko added 3 commits July 31, 2023 10:59

chore: mark TODO

9485602

feat: support effective gas price

5c23399

docs: solve TODO

07b6c22

johntaiko force-pushed the feat/evm_circuit_1559 branch from 75dc724 to 07b6c22 Compare July 31, 2023 15:01

johntaiko added 2 commits August 3, 2023 09:10

feat: mock with 1559 support and anchor support

9752cdb

fix: add l2 contract address into mock data

c973c78

Brechtpd reviewed Aug 3, 2023

View reviewed changes

bus-mapping/src/evm/opcodes/begin_end_tx.rs Outdated Show resolved Hide resolved

johntaiko added 3 commits August 7, 2023 09:48

feat: use taiko-geth for evm execution

61798cd

fix: duplicate symbol

d8800ab

fix: use taiko-geth

5e9573c

github-actions bot added crate-external-tracer crate-geth-utils crate-mock labels Aug 7, 2023

johntaiko added 6 commits August 7, 2023 12:50

fix: anchor without updating balance

4bc10f8

fix: update treasury with base_fee

68052dc

fix: use anchor flag in context

182c5e5

feat: treat anchor specially

03c2047

feat: treat anchor's gas_price as zero

aff5000

fix: testcases

917d836

github-actions bot added the crate-eth-types label Aug 8, 2023

Brechtpd reviewed Aug 9, 2023

View reviewed changes

smtmfft reviewed Aug 9, 2023

View reviewed changes

Update zkevm-circuits/src/evm_circuit/execution/end_tx.rs

ebe34f7

Co-authored-by: Brecht Devos <[email protected]>

johntaiko and others added 5 commits August 10, 2023 13:58

Update zkevm-circuits/src/evm_circuit/execution/end_tx.rs

848f896

Co-authored-by: Brecht Devos <[email protected]>

Update zkevm-circuits/src/evm_circuit/execution/end_tx.rs

42e25ae

Co-authored-by: Brecht Devos <[email protected]>

fix: comments in pr

4c208e6

chore: add FIXME

993d6da

fix: mock tx's signature with eip-1559

740e655

johntaiko added 3 commits August 10, 2023 07:31

fix: increase the step hight instead of width

9b64614

feat: add treasury account in the block table

8c33dc1

fix: taiko-mock's feature

11cdc1b

github-actions bot added crate-circuit-benchmarks T-bench labels Aug 10, 2023

johntaiko commented Aug 10, 2023

View reviewed changes

fix: need lookup the treasury

c6243e0

johntaiko mentioned this pull request Aug 11, 2023

Support eip-1559 and Evm-circuit in aggregation taikoxyz/zkevm-circuits#136

Merged

15 tasks

smtmfft approved these changes Aug 11, 2023

View reviewed changes

chore: remove TODO, support 1559 now

7175f7e

johntaiko closed this Aug 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: BeginTx and EndTx support taiko's 1559 #18

feat: BeginTx and EndTx support taiko's 1559 #18

johntaiko commented Jul 31, 2023 •

edited

Loading

Brechtpd left a comment

Brechtpd left a comment

Brechtpd Aug 8, 2023

johntaiko Aug 10, 2023 •

edited

Loading

Brechtpd Aug 11, 2023

Brechtpd Aug 8, 2023

smtmfft Aug 9, 2023

Brechtpd Aug 9, 2023

smtmfft Aug 9, 2023

Brechtpd Aug 9, 2023 •

edited

Loading

johntaiko Aug 10, 2023 •

edited

Loading

johntaiko Aug 10, 2023

Brechtpd Aug 9, 2023

johntaiko Aug 10, 2023

smtmfft Aug 9, 2023

smtmfft commented Aug 10, 2023

johntaiko commented Aug 10, 2023

johntaiko commented Aug 10, 2023

johntaiko Aug 10, 2023

Brechtpd commented Aug 11, 2023

johntaiko commented Aug 11, 2023

		pub const N_BYTE_LOOKUPS: usize = 24;
		pub const N_BYTE_LOOKUPS: usize = 28;

feat: BeginTx and EndTx support taiko's 1559 #18

feat: BeginTx and EndTx support taiko's 1559 #18

Conversation

johntaiko commented Jul 31, 2023 • edited Loading

Brechtpd left a comment

Choose a reason for hiding this comment

Brechtpd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johntaiko Aug 10, 2023 • edited Loading

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

Brechtpd Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

johntaiko Aug 10, 2023 • edited Loading

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

smtmfft commented Aug 10, 2023

johntaiko commented Aug 10, 2023

johntaiko commented Aug 10, 2023

Choose a reason for hiding this comment

Brechtpd commented Aug 11, 2023

johntaiko commented Aug 11, 2023

johntaiko commented Jul 31, 2023 •

edited

Loading

johntaiko Aug 10, 2023 •

edited

Loading

Brechtpd Aug 9, 2023 •

edited

Loading

johntaiko Aug 10, 2023 •

edited

Loading