-
Notifications
You must be signed in to change notification settings - Fork 148
core: add hardcoded transaction hash to gas used map #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
add map to store transaction hash along with its gas used to bypass transaction execution for problematic txs execution on ARM architecture Signed-off-by: Igor Braga <[email protected]>
core/hardcoded_tx_gas.go
Outdated
|
||
// HardcodedTxGasUsed maps specific transaction hashes to the amount of GAS used by that | ||
// specific transaction alone. | ||
var HardcodedTxGasUsed = map[common.Hash]uint64{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's have something in the name about it being a reverted transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in ae8da64
Signed-off-by: Igor Braga <[email protected]>
Signed-off-by: Igor Braga <[email protected]>
Signed-off-by: Igor Braga <[email protected]>
// Check against hardcoded transaction hashes that have previously reverted, so instead | ||
// of executing the transaction we just update state nonce and remaining gas to avoid | ||
// state divergence. | ||
usedMultiGas, vmerr = st.handleRevertedTx(msg, usedMultiGas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra level of indirection (function call) okay in this case? Or should we just inline it all? I'm worried about performance, given for every tx execution we introduced 3 checks:
// 1. check if mg.Tx is not nil
if msg.Tx == nil {
// 2. check if tx is inside RevertedTxGasUsed map
if l2GasUsed, ok := RevertedTxGasUsed[txHash]; ok {
// 3. make sure we continue on critical path if vmerr still nil
if vmerr == nil {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a way to reduce those checks to just one for the critical path. Instead of doing the above, we make 2 changes:
- Instead of having a map of
TxHash
->L2GasUsed
we would have a mapBlockNumber
->(TxHash, L2GasUsed)
- Instead of doing the check before
if contractCreation {
we guardst.evm.Call
only
That's only possible because we can get the block number from st.evm.Context.BlockNumber
without a nil check. The problem with this approach is that not every transaction inside the target block number are considered reverted transactions, so we need to call st.evm.Call
for those other transactions; and I think that's okay since that would just happen for the entries found inside RevertedTxGasUsed
. On the other hand, we introduce just one map lookup to all other transactions from the other blocks instead of adding a function call and a couple if checks.
The resulting code would look something like the following:
- map gets updated to:
var RevertedTxGasUsed = map[uint64]RevertedTxEntry{
204060366: {
TxHash: common.HexToHash("0x58df300a7f04fe31d41d24672786cbe1c58b4f3d8329d0d74392d814dd9f7e40"),
L2GasUsed: 45606,
},
}
- and the only logic in
state_transition
that gets updated would be aroundst.evm.Call
:
if revTxEntry, ok := RevertedTxGasUsed[st.evm.Context.BlockNumber.Uint64()]; ok {
ret, st.gasRemaining, multiGas, vmerr = st.handleRevertedTx(revTxEntry, msg, usedMultiGas, value)
} else {
ret, st.gasRemaining, multiGas, vmerr = st.evm.Call(msg.From, st.to(), msg.Data, st.gasRemaining, value)
}
where handleRevertedTx
would look like:
func (st *stateTransition) handleRevertedTx(revTxEntry RevertedTxEntry, msg *Message, multiGas multigas.MultiGas, value *uint256.Int) (ret []byte, leftOverGas uint64, retUsedMultiGas multigas.MultiGas, err error) {
if msg.Tx == nil {
return []byte{}, st.gasRemaining, multiGas, nil
}
txHash := msg.Tx.Hash()
if revTxEntry.TxHash == txHash {
adjustedGas := revTxEntry.L2GasUsed - params.TxGas
gasRemaining := st.gasRemaining - adjustedGas
return []byte{}, gasRemaining, multiGas, vm.ErrExecutionReverted
} else {
// A block might contain more than one transaction, therefore we need to make sure we execute such
// transactions in case they're not considered reverted.
return st.evm.Call(msg.From, st.to(), msg.Data, st.gasRemaining, value)
}
}
let me know if you prefer this new approach or the original one @tsahee
part of NIT-4027
pulled-by OffchainLabs/nitro#3876
Adds map to store transaction hash along with its gas used to bypass transaction execution for problematic txs execution on ARM architecture.