Skip invalid tx #32
Skip invalid tx #32
Conversation
xiaodino
changed the title
xiaodino
changed the title
xiaodino
changed the title
xiaodino
changed the title
Update witness generation for invalid tx
| [step.rw_indices[7], step.rw_indices[8], step.rw_indices[9]] | ||
| .map(|idx| block.rws[idx].account_value_pair()); | ||
| let [caller_nonce_pair, caller_balance_pair, callee_balance_pair, (callee_code_hash, _)] = | ||
| [4, 7, 8, 9].map(|idx| block.rws[step.rw_indices[idx]].account_value_pair()); |
There was a problem hiding this comment.
@Brechtpd Where are [4, 7, 8, 9] from?
bus-mapping/src/evm/opcodes.rs
Outdated
| let nonce_prev = state.sdb.get_account(&caller_address).1.nonce.as_u64(); | ||
| // Increase caller's nonce when the tx is not invalid | ||
| let nonce = if !state.tx.invalid_tx { | ||
| state.sdb.increase_nonce(&caller_address) + 1 |
There was a problem hiding this comment.
I think this line should be nonce_prev+1
There was a problem hiding this comment.
If you look at the implementation I think that's what this does, because increase_nonce, a bit surprisingly, returns the old nonce, not the new nonce.
| @@ -304,7 +304,8 @@ impl<'a> CircuitInputStateRef<'a> { | |||
| // AccountNonExisting proofs lookups). | |||
| if (!matches!(op.field, AccountField::CodeHash) | |||
| && (matches!(rw, RW::READ) || (op.value_prev.is_zero() && op.value.is_zero()))) | |||
| && account.is_empty() && !self.tx.invalid_tx | |||
| && account.is_empty() | |||
| && !self.tx.invalid_tx | |||
There was a problem hiding this comment.
For invalid transactions we also need to read the balance/nonce which could from a non-existing account, so I don't think we can just disable the check here? Or are those reads prevented in some other way?
There was a problem hiding this comment.
The current also did account balance reads before doing the codehash check, it seems that is incorrect: privacy-scaling-explorations#1118 (comment).
Let's way a bit until these things are fixed on the master before updating the code there.
| // state.sdb.increase_nonce(&caller_address) + 1 | ||
| nonce_prev + 1 |
There was a problem hiding this comment.
If the transaction is valid the nonce in the account state should be increased right? Could you explain more why we shouldn't update the nonce in the account for valid transactions they way it was done before (this code changes the behavior for normal transactions)?
There was a problem hiding this comment.
Ah never mind, I see the code on the master has been updated to not use increase_nonce either.
* Add enable_skipping_invalid_tx * Revert unused changes in zkevm-circuits/src/witness/tx.rs * Update * Update * Update * Update
| @@ -20,6 +20,7 @@ import ( | |||
| // while replaying a transaction in debug mode as well as transaction | |||
| // execution status, the amount of gas used and the return value | |||
| type ExecutionResult struct { | |||
| Invalid bool `json:"invalid"` | |||
There was a problem hiding this comment.
Invalid is not a field in github.com/ethereum/go-ethereum/internal/ethapi.ExecutionResult
There was a problem hiding this comment.
That's okay, this is mainly only there to make things a bit cleaner and to handle error messages better. If we run a trace against a real node and it errors our or returns nothing we'll also know it's invalid.
Please see #115 and #104