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

Eip1234 #17675

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Eip1234 #17675

merged 3 commits into from
Sep 17, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Sep 15, 2018

Replaces #17657 (but maintains the commit history from @eosclassicteam ). I wasn't sure you wanted me to push directly into your branch -- this PR contains a bit less copy-paste.

@holiman holiman requested a review from karalabe as a code owner September 15, 2018 21:49
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Instead of adding numbers for bomb delay, is it possible to "defuse" the bomb with exponential factor removal??

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@holiman I would like to suggest generalizing block rewards and diff calculation like parity did openethereum/parity-ethereum#9480 instead of copy-paste or forcing one rule for side chains. In this way it would be possible to tune block time and fork numbers on dev networks


// calcDifficultyConstantinople is the difficulty adjustment algorithm for Constantinople.
// It returns the difficulty that a new block should have when created at time given the
// parent block's time and difficulty. The calculation uses the Byzantium rules, but with bomb offset 5M.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, please move bomb offset 5M to a new line.

I'd also perhaps use 5M and 3M instead and do the decrement internally. It's not really that expensive imho but the code would look a lot cleaner opposed to guessing why the -1 here opposed to the EIPs.

Also please add a link to the two iceage postponing EIPs here for reference. Some future maintainer will thank us.

// Note, the calculations below looks at the parent number, which is 1 below
// the block number. Thus we remove one from the delay given
bombDelayFromParent := new(big.Int)
bombDelayFromParent = bombDelay.Sub(bombDelay, big1)
Copy link
Member

Choose a reason for hiding this comment

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

bombDelayFromParent := new(big.Int).Sub(bombDelay, big1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, done

@karalabe
Copy link
Member

@eosclassicteam While in general that's not a bad idea, within Geth we kind of always focused on the Ethereum use case and tried to avoid making everything too abstract/flexible. There have been multiple consensus issues in Parity caused by EIPs getting accidentally activated due to their flexible architecture, as well as configuring a Parity node is a nightmare that can lately only be done via custom tooling. Our code mostly assumes that Ethereum hard forks come in progression one after the other and this keeps everything a lot simpler. I of course understand that this puts a bit more burden on teams creating Ethereum forks, but at the end of the day why should the go-ethereum maintainers pay the price of running derivatives based on our code? :)

@karalabe
Copy link
Member

For dev/private networks we already have the Clique engine, so PoW difficulty/rewards are a lot less relevant.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants