Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: remove ineffective witness assignment #463

Open
wants to merge 1 commit into
base: kaustinen-with-shapella
Choose a base branch
from
Open

fix: remove ineffective witness assignment #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion core/chain_makers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,6 @@ func GenerateVerkleChain(config *params.ChainConfig, parent *types.Block, engine
if err != nil {
panic(fmt.Sprintf("could not find state for block %d: err=%v, parent root=%x", i, err, parent.Root()))
}
statedb.NewAccessWitness()
block, receipt := genblock(i, parent, statedb)
blocks[i] = block
receipts[i] = receipt
Expand Down
2 changes: 1 addition & 1 deletion core/state/statedb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?)

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Suggested change
panic("witness wasn't initialized")
// TODO this is meant to catch rebase errors, replace with error return when stabilized
panic("witness wasn't initialized")

}
return s.witness
}
Expand Down
3 changes: 0 additions & 3 deletions core/vm/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,6 @@ func NewEVM(blockCtx BlockContext, txCtx TxContext, statedb StateDB, chainConfig
chainConfig: chainConfig,
chainRules: chainConfig.Rules(blockCtx.BlockNumber, blockCtx.Random != nil, blockCtx.Time),
}
if txCtx.Accesses == nil && chainConfig.IsPrague(blockCtx.BlockNumber, blockCtx.Time) {
evm.Accesses = evm.StateDB.(*state.StateDB).NewAccessWitness()
}
Comment on lines -140 to -142
Copy link
Owner Author

Choose a reason for hiding this comment

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

this stuff is recreated in applyTransaction and isn't needed before, so this is a redundant creation.

Copy link
Collaborator

@jsign jsign Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

evm.interpreter = NewEVMInterpreter(evm)
return evm
}
Expand Down
Loading