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: forkchoice.go more readable code #26257

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

setunapo
Copy link
Contributor

no logic change

@holiman
Copy link
Contributor

holiman commented Nov 25, 2022

Myeah, I can verify that your change is ok (does what it says). However, if we're clarifying things, perhaps this would be an improvement too?

	// If the total difficulty is higher than our known, add it to the canonical chain
	if diff := externTd.Cmp(localTD); diff > 0 {
		return true, nil
	} else if diff < 0 {
		return false, nil
	}
	// Local and external difficulty is identical.
	// Second clause in the if statement reduces the vulnerability to selfish mining.
	// Please refer to http://www.cs.cornell.edu/~ie53/publications/btcProcFC.pdf
	externNum, localNum := extern.Number.Uint64(), current.Number.Uint64()
	if externNum < localNum {
		reorg = true
	} else if externNum == localNum {
		var currentPreserve, externPreserve bool
		if f.preserve != nil {
			currentPreserve, externPreserve = f.preserve(current), f.preserve(extern)
		}
		reorg = !currentPreserve && (externPreserve || f.rand.Float64() < 0.5)
	}
	return reorg, nil

OTOH, not sure it's worth messing with this code -- in a post-merge world, the difficulty is not all that important any more.

@brilliant-lx
Copy link

brilliant-lx commented Nov 25, 2022

Myeah, I can verify that your change is ok (does what it says). However, if we're clarifying things, perhaps this would be an improvement too?

	// If the total difficulty is higher than our known, add it to the canonical chain
	if diff := externTd.Cmp(localTD); diff > 0 {
		return true, nil
	} else if diff < 0 {
		return false, nil
	}
	// Local and external difficulty is identical.
	// Second clause in the if statement reduces the vulnerability to selfish mining.
	// Please refer to http://www.cs.cornell.edu/~ie53/publications/btcProcFC.pdf
	externNum, localNum := extern.Number.Uint64(), current.Number.Uint64()
	if externNum < localNum {
		reorg = true
	} else if externNum == localNum {
		var currentPreserve, externPreserve bool
		if f.preserve != nil {
			currentPreserve, externPreserve = f.preserve(current), f.preserve(extern)
		}
		reorg = !currentPreserve && (externPreserve || f.rand.Float64() < 0.5)
	}
	return reorg, nil

OTOH, not sure it's worth messing with this code -- in a post-merge world, the difficulty is not all that important any more.

Yea, your post if fine, making it much clearer, I will update.
After merge, difficulty is still important, it represents inturn & offturn.
It always necessary to make code clear, for easy maintenance, imo.
And there are many code in Geth could be improved to make it more understandable.

@holiman holiman added this to the 1.11.0 milestone Nov 28, 2022
@holiman holiman merged commit 0dc9b01 into ethereum:master Nov 28, 2022
@holiman
Copy link
Contributor

holiman commented Nov 28, 2022

Thanks!

shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
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