Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ECIP-1039: Monetary policy rounding specification #7067

Merged
merged 4 commits into from
Nov 24, 2017
Merged

ECIP-1039: Monetary policy rounding specification #7067

merged 4 commits into from
Nov 24, 2017

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Nov 16, 2017 •
edited
Loading

(ethereumproject/ECIPs#83 is accepted. Okay to merge.)

Fix potential rounding errors between geth and parity in the long-term future.

@sorpaas
ECIP-1039: Monetary policy rounding specification
2ca57d7 
Fix potential rounding errors between geth and parity in the long-term future.
@parity-cla-bot
Copy link

It looks like @sorpaas signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. labels Nov 16, 2017
@5chdn 5chdn added this to the 1.9 milestone Nov 16, 2017
@whilei
Copy link
Contributor

whilei commented Nov 16, 2017

My Rust is rusty... I guess in regard to uncle rewards that shr, eg reward.shr(5) in https://github.com/sorpaas/parity/blob/b35cc3f7168d03b7c22bf462b774ee1fa3e5fc8d/ethcore/src/ethereum/ethash.rs#L222, means reward*(2^-5) == reward/32?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Nov 16, 2017

@whilei Yes.

@whilei
Copy link
Contributor

whilei commented Nov 16, 2017 •
edited
Loading

onice, LGTM

@whilei whilei mentioned this pull request Nov 16, 2017
Merged
@whilei
Copy link
Contributor

whilei commented Nov 24, 2017

Just a heads up -- final rounding specs merged via ethereumproject/ECIPs#83.

Proposed changes are cleared for 🛫

@debris
Copy link
Collaborator

debris commented Nov 24, 2017

@sorpaas can you also add tests to has_valid_ecip1017_eras_block_reward validating new behaviour?

@whilei
Copy link
Contributor

whilei commented Nov 24, 2017 •
edited
Loading

A5-grumble A9-buythatmanabeer @debris Can be a job for your team?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Nov 24, 2017

@debris @whilei I'll do it.

debris
debris approved these changes Nov 24, 2017
View reviewed changes
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

lgtm, just one minor grumble! :)

ethcore/src/ethereum/ethash.rs Outdated
let block_number = 250000000;
let (eras, reward) = ecip1017_eras_block_reward(eras_rounds, start_reward, block_number);
assert_eq!(48, eras);
assert!(U256::from_str("65697BFA9ACB").unwrap() != reward);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line is redundant

@whilei whilei mentioned this pull request Nov 24, 2017
Closed
@sorpaas
Wrong era used in ECIP1017 test
8e89be7 
It is era 49, and should correspond to ECIP1017/ECIP1039's era 50.
@sorpaas
Copy link
Collaborator Author

sorpaas commented Nov 24, 2017

@debris The CI failure seems unrelated. I ran the test locally and it's passing. Can you restart it?

@debris
Copy link
Collaborator

debris commented Nov 24, 2017

@sorpaas restarted, I will merge the pr as soon as it is green

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Nov 24, 2017
@debris debris merged commit 68db425 into openethereum:master Nov 24, 2017
tomusdrw pushed a commit that referenced this pull request Dec 4, 2017
@debris @tomusdrw
Merge pull request #7067 from sorpaas/patch-1
0ec35d9 
ECIP-1039: Monetary policy rounding specification
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@debris debris debris approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
1.9 Velocity
Development

Successfully merging this pull request may close these issues.
6 participants
@sorpaas @parity-cla-bot @whilei @debris @tomusdrw @5chdn