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

cmd/utils: require TTD and difficulty to be zero at genesis for dev mode #29579

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

fselmo
Copy link
Contributor

@fselmo fselmo commented Apr 18, 2024

Related to #29469 as that doesn't seem resolved if the user sets their own genesis config. I also discovered TTD can be negative and I could trick the correct state by setting TTD = -1 and difficulty = 0 since 0 difficulty is what triggers the correct EVM rules. This PR also validates that negative values are not accepted.


If genesis config is set by the user, difficulty must be > TTD. However, if TTD is 0 and we are post-merge, setting difficulty > 0 will use the wrong EVM fork rules and raise issues like:

WARN [04-18|09:34:02.022] Served eth_estimateGas                   reqid=3 duration="419.912µs" err="invalid opcode: PUSH0"

I'm not sure if this is the best way to resolve it but if the user can set both to 0 at genesis, that makes sense that we'd be in a state where the difficulty is 0 and we are at TTD - which seems to be required for correct EVM rules. Perhaps a better message is needed for when TTD is set to 0 and difficulty is >0. As of right now, that would just end up using incorrect EVM rules which might be confusing to the user.

I can confirm this works as expected for me. Setting TTD = 0 and difficulty = 0, I get correct EVM rules. I can also no longer set TTD = -1 and difficulty = 0 to trick the EVM rules to trigger.

Thoughts?


edit: I think a better UX may be to only allow TTD = 0 and difficulty = 0 for dev mode (see my comment below). If that's the case I can force push the change here.

Note: I've updated the code to be this latter option instead of validating any other values for TTD and difficulty.

@fselmo
Copy link
Contributor Author

fselmo commented Apr 18, 2024

Alternatively, to get rid of any ambiguity, since it is already the case that terminalTotalDifficultyPassed must be true in developer mode:

if genesis.Config.TerminalTotalDifficulty.Cmp(big.NewInt(0)) != 0 {
	Fatalf("Bad developer-mode genesis configuration: terminalTotalDifficulty must be 0 in developer mode")
}
if genesis.Difficulty.Cmp(big.NewInt(0)) != 0 {
	Fatalf("Bad developer-mode genesis configuration: difficulty must be 0 in developer mode")
}

This seems to be reasonable and probably the best UX out of the two options. Unless there's a good reason to allow arbitrary TTD and difficulty in dev mode. It seems like the difficulty will have to be reset to 0 anyhow to trigger the correct EVM rules as it stands. Perhaps TTD can be validated that it is any arbitrary number above 0, but difficulty should always be 0 if we are post-merge.

@fselmo fselmo changed the title Disallow negative difficulty values; allow 0 TTD and difficulty @ genesis Disallow negative difficulty values; allow 0 TTD and 0 difficulty @ genesis Apr 18, 2024
@jwasinger
Copy link
Contributor

Alternatively, to get rid of any ambiguity, since it is already the case that terminalTotalDifficultyPassed must be true in developer mode:

if genesis.Config.TerminalTotalDifficulty.Cmp(big.NewInt(0)) != 0 {
	Fatalf("Bad developer-mode genesis configuration: terminalTotalDifficulty must be 0 in developer mode")
}
if genesis.Difficulty.Cmp(big.NewInt(0)) != 0 {
	Fatalf("Bad developer-mode genesis configuration: difficulty must be 0 in developer mode")
}

This seems to be reasonable and probably the best UX out of the two options. Unless there's a good reason to allow arbitrary TTD and difficulty in dev mode. It seems like the difficulty will have to be reset to 0 anyhow to trigger the correct EVM rules as it stands.

I added the same validation in #29566 but opted to close it. My thinking is that there is no valid reason for a user to change these. If we go the route of trying to catch every possible misconfiguration that a user could make, we will end up with bloated validation logic that hurts readability.

@fselmo
Copy link
Contributor Author

fselmo commented Apr 18, 2024

Ah, well. It's fairly difficult to discern what's going on currently if those values are set since even though shanghaiTime and cancunTime are turned on, PUSH0 is not available. I think perhaps some sort of validation that the user can't set them might be helpful? But obviously I'll leave that decision for y'all to decide. I can only speak to the user experience definitely being better with these stricter guidelines in place. Thanks for the quick response 👍🏼


edit: With a custom genesis, I don't believe it's possible to leave these values alone also

@fselmo fselmo force-pushed the allow-0-difficulty-if-ttd-is-0 branch from ab25969 to 69fc89d Compare April 18, 2024 18:08
@fselmo fselmo changed the title Disallow negative difficulty values; allow 0 TTD and 0 difficulty @ genesis Require TTD and difficulty to be 0 at genesis for dev mode Apr 18, 2024
cmd/utils/flags.go Outdated Show resolved Hide resolved
@fselmo fselmo force-pushed the allow-0-difficulty-if-ttd-is-0 branch from 69fc89d to 56bf23e Compare April 18, 2024 18:21
@jwasinger
Copy link
Contributor

I would retain the genesis.Config.TerminalTotalDifficulty == nil check above where we check that it is 0. but otherwise, LGTM.

@fselmo fselmo force-pushed the allow-0-difficulty-if-ttd-is-0 branch 2 times, most recently from 1676e39 to 1477862 Compare April 18, 2024 18:40
@fselmo
Copy link
Contributor Author

fselmo commented Apr 18, 2024

I would retain the genesis.Config.TerminalTotalDifficulty == nil check above where we check that it is 0. but otherwise, LGTM.

Changed to keep null pointer check 👍🏼

cmd/utils/flags.go Outdated Show resolved Hide resolved
- No need to reiterate that this is for developer mode since it's in the error already.
@fselmo fselmo force-pushed the allow-0-difficulty-if-ttd-is-0 branch from 1477862 to 0a0e1fe Compare April 22, 2024 03:25
@fjl fjl changed the title Require TTD and difficulty to be 0 at genesis for dev mode cmd/utils: require TTD and difficulty to be zero at genesis for dev mode Apr 30, 2024
@fjl fjl added this to the 1.14.1 milestone Apr 30, 2024
@fjl fjl merged commit c04b8e6 into ethereum:master Apr 30, 2024
3 checks passed
@fselmo fselmo deleted the allow-0-difficulty-if-ttd-is-0 branch April 30, 2024 15:02
corverroos added a commit to omni-network/omni that referenced this pull request May 9, 2024
`go: upgraded github.com/ethereum/go-ethereum v1.14.0 => v1.14.2`

Also update geth genesis difficulty to 0 as per [geth
PR](ethereum/go-ethereum#29579).

task: none
Copy link

@gatleas17 gatleas17 left a comment

Choose a reason for hiding this comment

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

Thx

jorgemmsilva pushed a commit to iotaledger/go-ethereum that referenced this pull request Jun 17, 2024
stwiname pushed a commit to subquery/data-node-go-ethereum that referenced this pull request Sep 9, 2024
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.

7 participants