search

poanetwork / parity-ethereum

Fast, light, robust Ethereum implementation.
https://parity.io
Other
10 stars 12 forks source link

posdaoTransition spec option #151

Closed vkomenda closed 5 years ago

vkomenda commented 5 years ago

Fixes #141.

This PR can be tested with https://github.com/poanetwork/posdao-test-setup/tree/posdao-transition

varasev commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 :arrow_up:

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

varasev commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

varasev commented 5 years ago

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

Ok, thanks

varasev commented 5 years ago

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 commented 5 years ago

Let's also add an info level log message as we did for the quorum_2_3_transition: https://github.com/poanetwork/parity-ethereum/pull/109/commits/aa7dce71ae26e0c03765d09f4de3d31d3fedace2#diff-3b2a71ed5fccb75dfe326f3b122487cd

vkomenda commented 5 years ago

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 commented 5 years ago

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

vkomenda commented 5 years ago

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