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

core/forkid: skip genesis forks by time #28034

Merged
merged 4 commits into from
Sep 4, 2023

Conversation

lightclient
Copy link
Member

I made a mistake in #27895, I didn't realize newFilter also generated the fork id hash. This caused geth to disconnect from peers during the handshake. I've resolved it by modifying gatherForks to simply skip forks that are non-zero and before the genesis timestamp.

@holiman
Copy link
Contributor

holiman commented Aug 31, 2023

I think the API is a bit clunky.

func NewID(config *params.ChainConfig, genesis *types.Block, head, time uint64) ID 

The genesis is a *types.Block, but really all we need is *types.Header, to internally check Hash() and Time()
The head, time are uint64, but in reality, nearly [1] all callers have a head *types.Header already, and picks out the individual fields to pass in.

IMO a better api would be

func NewID(config *params.ChainConfig, genesis, head *types.Header) ID 

[1]: The callsite cmd/devp2p/internal/ethtest/chain.go does something very wonky, passing in the genesis time:

// ForkID gets the fork id of the chain.
func (c *Chain) ForkID() forkid.ID {
	return forkid.NewID(c.chainConfig, c.blocks[0], uint64(c.Len()), c.blocks[0].Time())
}

It looks like it should be

// ForkID gets the fork id of the chain.
func (c *Chain) ForkID() forkid.ID {
	return forkid.NewID(c.chainConfig, c.blocks[0], uint64(c.Len()), c.blocks[c.Len() -1].Time())
}

Just the type of error that would not have happened with my suggested api :)
Don't have to be done in this PR, if this is a bugfix.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

You also added a genesis.json file, is that intentional?

core/forkid/forkid.go Outdated Show resolved Hide resolved
core/forkid/forkid.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Aug 31, 2023

I think the API is a bit clunky.

@karalabe says to leave it as is (triage discussion)

@lightclient
Copy link
Member Author

You also added a genesis.json file, is that intentional?

not intentional! removed

@lightclient
Copy link
Member Author

Should be okay now. Sorry for all the back and forth, brain was a little fried last week.
image

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.13.0 milestone Sep 4, 2023
@holiman holiman merged commit eff7c3b into ethereum:master Sep 4, 2023
1 check passed
@ismaelapariciojr
Copy link

ismaelapariciojr commented Sep 4, 2023 via email

devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
* core/forkid: skip genesis forks by time

* core/forkid: add comment about skipping non-zero fork times

* core/forkid: skip all time based forks in genesis using loop

* core/forkid: simplify logic for dropping time-based forks
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@ismaelapariciojr
Copy link

wallet_2

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.

4 participants