fix: remove ineffective witness assignment #463
Conversation
| if txCtx.Accesses == nil && chainConfig.IsPrague(blockCtx.BlockNumber, blockCtx.Time) { | ||
| evm.Accesses = evm.StateDB.(*state.StateDB).NewAccessWitness() | ||
| } |
There was a problem hiding this comment.
this stuff is recreated in applyTransaction and isn't needed before, so this is a redundant creation.
There was a problem hiding this comment.
I've checked all references to NewEVM and there're many places that call this method that aren't applyTransaction.
I feel it would be much safer to maybe move that initialization inside NewEVMTxContext(...)? I'm not sure why it makes sense to initialize one of its attributes outside the ctor creating these kind of situations.
Also noted that that referenced line doesn't do the chainConfig.IsPrague(blockCtx.BlockNumber, blockCtx.Time) style check that we did here. I guess that's just creating a little extra garbage for block ranges that the witness doesn't apply, but just mentioning.
If we're always fine with doing it ignoring the right fork (as done today in applyTransaction), then moving it to NewEVMTxContext is possible since that method doesn't have block num.
| s.witness = s.NewAccessWitness() | ||
| panic("witness wasn't initialized") |
There was a problem hiding this comment.
I had the feeling this removed logic was intentionally trying to lazily initialize in some cases.
Thing is that if we remove this, we could panic "for a while" but eventually this method will have to change to returning *AccessWitnes, error. (Or avoid the if check?)
There was a problem hiding this comment.
yes the if check should not be necessary, I am just setting the panic to catch a rebase issue. I'll add a TODO here.
| if txCtx.Accesses == nil && chainConfig.IsPrague(blockCtx.BlockNumber, blockCtx.Time) { | ||
| evm.Accesses = evm.StateDB.(*state.StateDB).NewAccessWitness() | ||
| } |
There was a problem hiding this comment.
I've checked all references to NewEVM and there're many places that call this method that aren't applyTransaction.
I feel it would be much safer to maybe move that initialization inside NewEVMTxContext(...)? I'm not sure why it makes sense to initialize one of its attributes outside the ctor creating these kind of situations.
Also noted that that referenced line doesn't do the chainConfig.IsPrague(blockCtx.BlockNumber, blockCtx.Time) style check that we did here. I guess that's just creating a little extra garbage for block ranges that the witness doesn't apply, but just mentioning.
If we're always fine with doing it ignoring the right fork (as done today in applyTransaction), then moving it to NewEVMTxContext is possible since that method doesn't have block num.
| @@ -192,7 +192,7 @@ func (s *StateDB) NewAccessWitness() *AccessWitness { | |||
|
|
|||
| func (s *StateDB) Witness() *AccessWitness { | |||
| if s.witness == nil { | |||
| s.witness = s.NewAccessWitness() | |||
| panic("witness wasn't initialized") | |||
There was a problem hiding this comment.
| panic("witness wasn't initialized") | |
| // TODO this is meant to catch rebase errors, replace with error return when stabilized | |
| panic("witness wasn't initialized") |
I noticed that there was an ineffectual witness creation, as well as a misplaced check for the existence of the witness, I am removing them for cleanup - they clash with some changes introduced by FILL_COST