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.

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

Upgrade EVM to Cancun #97

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Upgrade EVM to Cancun #97

wants to merge 28 commits into from

Conversation

khoau2u
Copy link
Contributor

@khoau2u khoau2u commented May 8, 2024

Please check if what you want to add to go-u2u list meets Contribution guidelines, maintainers note and Quality standard.

@khoau2u khoau2u self-assigned this May 8, 2024
@khoau2u khoau2u requested review from yadsendew, b1m0n and lewtran and removed request for yadsendew May 8, 2024 07:23
@khoau2u khoau2u changed the title Upgrade/cancun evm Upgrade EVM to Cancun May 8, 2024
@khoau2u
Copy link
Contributor Author

khoau2u commented May 16, 2024

I see that we have difficulty to validate this PR. Here are what is exactly included in this PR:

  • EIP-1153 Transient storage opcodes
  • EIP-3855 PUSH0
  • EIP-3860 Limit and meter initcode
  • EIP-4844 Blob transaction, precompiles and default trusted setup
  • EIP-5656 MCOPY
  • EIP-6780 SELFDESTRUCT only in same transaction
  • EIP-7516 BLOBBASEFEE
  • INVALID, RANDAO (zeroed) opcode
  • Upgrade some dependencies away from vulnerable versions

References:

Copy link
Contributor

@b1m0n b1m0n left a comment

Choose a reason for hiding this comment

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

So far this PR lacks of unit tests, for example transitioning from pre-EIP712 to post-EIP712 simulation.

Another big missing is the random value which is serving PREVRANDAO opcode is not properly created in EVM context. Following go-ethereum, this value is calculated each block below

if header.Difficulty.Sign() == 0 {
    random = &header.MixDigest
}

Also could you declare why we need the RANDOM opcode? I see that it's totally duplicated with PREVRANDAO

AA.md Outdated Show resolved Hide resolved
crypto/kzg4844/kzg4844.go Outdated Show resolved Hide resolved
core/vm/opcodes.go Outdated Show resolved Hide resolved
params/protocol_params.go Outdated Show resolved Hide resolved
params/protocol_params.go Outdated Show resolved Hide resolved
core/vm/contracts.go Outdated Show resolved Hide resolved
@khoau2u khoau2u changed the base branch from master to ft/paymaster_flow May 24, 2024 12:10
@khoau2u khoau2u changed the base branch from ft/paymaster_flow to master May 24, 2024 12:13
@khoau2u
Copy link
Contributor Author

khoau2u commented May 24, 2024

Updated as requested.
Pending full tests.

@khoau2u khoau2u requested a review from b1m0n June 8, 2024 11:49
@khoau2u khoau2u linked an issue Aug 3, 2024 that may be closed by this pull request
}), db, n, gen)
}

func TestEVMTransition(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reckon that the TSTORE logic is simple enough that more integration tests are needed instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure we need more integration tests, but the priciple of unit testing is trying to cover more cases. These two method of testing should take different repsonsibilities. So we need to cover as much cases we found out for more code coveraged, as well as detect clashing with future changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, double check is never enough, also prevent breaking changes from passing unnoticed afterward

@trinhdn97
Copy link
Contributor

trinhdn97 commented Oct 29, 2024

So far this PR lacks of unit tests, for example transitioning from pre-EIP712 to post-EIP712 simulation.

Another big missing is the random value which is serving PREVRANDAO opcode is not properly created in EVM context. Following go-ethereum, this value is calculated each block below

if header.Difficulty.Sign() == 0 {
    random = &header.MixDigest
}

Also could you declare why we need the RANDOM opcode? I see that it's totally duplicated with PREVRANDAO

We can calculate the PREVRANDAO hash by block events Fantom-foundation/Sonic#266

// GetPrevRandao returns the PrevRandao for current block.
func (b *Block) GetPrevRandao() common.Hash {
	if b.prevRandao == (common.Hash{}) {
		b.computePrevRandao()
	}
	return b.prevRandao
}

// computePrevRandao computes the PrevRandao from transaction hashes.
func (b *Block) computePrevRandao() {
	for _, event := range b.Events {
		for i := 0; i < 24; i++ {
			// first 8 bytes should be ignored as they are not pseudo-random.
			b.prevRandao[i+8] = b.prevRandao[i+8] ^ event[i+8]
		}
	}
}

@khoau2u
Copy link
Contributor Author

khoau2u commented Oct 30, 2024

@trinhdn97
We can consider this, but very few dapps use that opcode due to its known weakness (predictability, not manipulability).
Or we could use a timestamp, which wouldn't break the dapp but would be less secure for their rare case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade EVM to Cancun
4 participants