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

Use legacy transaction format and defer support of dynamic gas price#78

Merged
han0110 merged 7 commits into
privacy-ethereum:masterfrom
han0110:feature/use-legacy-tx-format
Jan 10, 2022
Merged

Use legacy transaction format and defer support of dynamic gas price#78
han0110 merged 7 commits into
privacy-ethereum:masterfrom
han0110:feature/use-legacy-tx-format

Conversation

@han0110
Copy link
Copy Markdown
Contributor

@han0110 han0110 commented Dec 8, 2021

This PR aims to change the transaction format to legacy one, and defer support of dynamic gas price transaction introduced by EIP1559 to the future work for simplicity.

It also does:

  • Extend add_word to add_words to match current circuit implementation
  • Add CallDataGasCost in tx_table for lookup to calculate the correct intrinsic gas cost for legacy tx

@han0110 han0110 changed the title Use legacy transaction format and defer EIP1559 Use legacy transaction format and defer support of dynamic gas price Dec 10, 2021
@han0110 han0110 force-pushed the feature/use-legacy-tx-format branch from d929532 to bd20b0d Compare December 31, 2021 08:55
@han0110 han0110 mentioned this pull request Jan 3, 2022
1 task
Comment thread src/zkevm_specs/evm/instruction.py Outdated
Copy link
Copy Markdown
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.

LTGM

@0xmountaintop 0xmountaintop mentioned this pull request Jan 7, 2022
gas: U64
gas_tip_cap: U256
gas_fee_cap: U256
gas_price: U256
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

does gas_price really need U256 size ?

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 am not sure actually, I basically tried to follow those types used in geth. Some of them like nonce and gas have less bits than the one specified in yellow paper, but others match.

So I was thinking there might be some reason for geth to have big.Int for gas_price, so I keep it as U256 here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if so, does N_BYTES_GAS = 8 of param.py need to change accordingly ?

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.

N_BYTES_GAS is for gas itself, tho it could have value up to 256 bits in yellow paper, but it's used as uint64 in geth, so N_BYTES_GAS = 8.

While gas_price is the unit that maps gas to real ether, which also could have value up to 256 bits, and used as big.Int in geth, so we try not to change it in case there is some edge cases that haven't came to our minds.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK to me for now! Thx

caller_balance = rlc_store.to_rlc(int(1e20) - (tx.value + tx.gas * tx.gas_price), 32)
callee_balance = rlc_store.to_rlc(tx.value, 32)

bytecode = Bytecode("00")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understand for begin_tx ,the byte code bytecode = Bytecode("00") here doesn't matter for any input , right ?

Copy link
Copy Markdown
Contributor Author

@han0110 han0110 Jan 9, 2022

Choose a reason for hiding this comment

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

Here is a decision I haven't figured it out yet, thanks for reminding me, I will convert this into a issue ASAP.

The decision is, if callee.code_hash = keccak256('') (e.g. callee is not yet a contract or precompiled), and when there is call to it, we have 2 options:

  1. Only do ether transfer, and not dive into another call.
  2. Do ether transfer, and prepare the call context just like we are diving into another call. The first executed opcode will always be STOP because program_counter == code_length (this check for opcode lookup is not yet implemented).

The first option is the exact behavior of geth (ref), but it introduces a branch if callee.code == keccak256(''), which requires some more constraints.

The second option is somehow weird, but it doesn't need extra constraint of BeginTx or CALL in the first option, by sacrificing an extra step, which I think is okay to do.

So the testing now have Bytecode("00"), which is actually having the exact behavior of the second option, to temporarily avoid this decision.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yeah, we can think more later, I can approve this pr first for now.

Copy link
Copy Markdown
Contributor

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@han0110 han0110 merged commit 352c6fd into privacy-ethereum:master Jan 10, 2022
@han0110 han0110 deleted the feature/use-legacy-tx-format branch January 10, 2022 12:34
@han0110 han0110 mentioned this pull request Jan 12, 2022
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants