[fix] skip value is zero in end_tx #1600
Conversation
github-actions
bot
added
the
crate-bus-mapping
There was a problem hiding this comment.
LGTM.
Let me try to explain if I understand the fix. The PR refactored the coinbase transfer to transfer_to_irreversible, which implemented the correct behavior of transferring to the coinbase.
Especially, when the transfer value is zero and the coinbase account doesn't exist, the coinbase account won't be created.
| @@ -277,10 +277,8 @@ fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Error> | |||
| if !found { | |||
| return Err(Error::AccountNotFound(state.block.coinbase)); | |||
| } | |||
| let coinbase_account = coinbase_account.clone(); | |||
| let coinbase_balance_prev = coinbase_account.balance; | |||
| let coinbase_exist = !coinbase_account.is_empty(); | |||
| let coinbase_transfer_value = effective_tip * (state.tx.gas() - exec_step.gas_left); | |||
There was a problem hiding this comment.
Do we know when will the coinbase_transfer_value be zero? Is it when the signature or nonce was bad or tx.gas_limit was set to zero?
There was a problem hiding this comment.
@lispc said according to spec, the gas price can be zero, then value is zero. though it won't happen in real world
There was a problem hiding this comment.
I made a wrong assumption that the coinbase_transfer_value will never be zero so in the previous pr I didn't include this
There was a problem hiding this comment.
@lispc said according to spec, the gas price can be zero, then value is zero.
Makes sense. Thanks for elaborating on it.
There was a problem hiding this comment.
LGTM! This should fix the error found in #1581
Please @lightsing remember to add some description in the PR in future occasions, it helps us understand the context of the PR for reviewing.
f36df6e
|
oh i think we should also change codes in evm_circuits? the assignment codes must be changed? failed #1581 |
Ah you're right! |
No description provided.