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

posdaoTransition spec option #151

Merged
merged 8 commits into from
Jun 26, 2019
Merged

posdaoTransition spec option #151

merged 8 commits into from
Jun 26, 2019

Conversation

vkomenda
Copy link

@vkomenda vkomenda force-pushed the vk-posdao-transition branch from 00ec497 to e12e388 Compare June 19, 2019 13:39
@vkomenda vkomenda marked this pull request as ready for review June 19, 2019 16:31
@vkomenda vkomenda requested review from afck, varasev and phahulin June 19, 2019 16:31
Copy link
Collaborator

@afck afck left a comment

Choose a reason for hiding this comment

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

Looks correct, but I haven't tested it.
Not sure what's the best approach to testing here: We can't expect the posdao-test-setup tests to pass before the POSDAO transition block, and I'm not sure the contracts are fully compatible in the first place?

@varasev
Copy link

varasev commented Jun 20, 2019

Not sure what's the best approach to testing here

I'll think about it. I will first check the code and unit tests, then try to launch it with poa-test-setup and test-block-reward without posdaoTransition option, and then test posdaoTransition activation.

@varasev
Copy link

varasev commented Jun 20, 2019

The code looks good, but I didn't yet test it with test setups. As far as I understand, the posdaoTransition is None by default, so for the unit tests it is switched off. Can we set posdaoTransition to 0 for the unit tests? (to activate it right from the genesis)

@vkomenda
Copy link
Author

Can we set posdaoTransition to 0 for the unit tests? (to activate it right from the genesis)

It is set to 0 in the integration test branch ⬆️

What unit tests are you thinking of? We might need to provide template POSDAO contracts in parity-ethereum.

@varasev
Copy link

varasev commented Jun 20, 2019

I mean cd ethcore && cargo test. I think we should add "posdaoTransition": 0 to ethcore/res/validator_contract.json or ethcore/src/snapshot/tests/test_validator_contract.json (not sure which one is correct).

@vkomenda
Copy link
Author

Those old tests assume None. If you want to test another setting then you are talking about adding new tests. We could have a unit tests for DOSDAO transition with appropriate template contracts in the parity-ethereum repo.

@varasev
Copy link

varasev commented Jun 20, 2019

No, I just mean that the unit tests worked with the activated posdao features by default before this PR. Now, since the posdaoTransition was introduced, the same unit tests work with deactivated posdao features. I'd just propose to activate posdao features for those unit tests as it was before this PR. We need to add "posdaoTransition": 0 in spec for them to make sure reportMalicious is not broken (we have a unit test for the reporting).

@vkomenda
Copy link
Author

I changed the transition to Some(0) in the unit test and it passed.

@varasev
Copy link

varasev commented Jun 20, 2019

I changed the transition to Some(0) in the unit test and it passed.

Ok, thanks

@varasev
Copy link

varasev commented Jun 20, 2019

We also need to only activate zero gas price for the reportMalicious transactions when posdaoTransition block is reached. Before the posdaoTransition block, gas price for reportMalicious should be non-zero like in upstream's implementation.

@varasev
Copy link

varasev commented Jun 21, 2019

Let's also add an info level log message as we did for the quorum_2_3_transition: aa7dce7#diff-3b2a71ed5fccb75dfe326f3b122487cd

Copy link

@varasev varasev left a comment

Choose a reason for hiding this comment

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

Works well but we need to add a couple of additions before merging to aura-pos: #151 (comment) and #151 (comment)

@vkomenda
Copy link
Author

Works well but we need to add a couple of additions before merging to aura-pos: #151 (comment) and #151 (comment)

Yes, I'll add those.

@vkomenda vkomenda force-pushed the vk-posdao-transition branch from e7f3ea1 to 4ee9e9e Compare June 25, 2019 13:07
@vkomenda
Copy link
Author

I've implemented the two additions. @varasev, can you please check if that's what you need?

ethcore/src/engines/authority_round/mod.rs Outdated Show resolved Hide resolved
ethcore/src/engines/authority_round/mod.rs Show resolved Hide resolved
ethcore/src/engines/validator_set/contract.rs Outdated Show resolved Hide resolved
ethcore/src/engines/validator_set/contract.rs Outdated Show resolved Hide resolved
@vkomenda
Copy link
Author

The PR is ready for testing. All the features are now implemented.

@varasev varasev self-requested a review June 26, 2019 13:53
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.

Add posdaoTransition option into engine.authorityRound.params of spec.json
4 participants