Page MenuHomePhabricator

Implement setting of UAHF start time via RPC
ClosedPublic

Authored by freetrader on Jun 21 2017, 15:14.

Details

Summary

Implement getuahfstarttime / setuahfstarttime RPC calls.

It is not allowed to update UAHF start time once the chain tip MTP exceeds
the existing configured start time (i.e. the fork is activated).

It is also not allowed to set it to a value less than 2 hours ahead of current
chain tip MTP.

Test Plan

make check
qa/pull-tester/rpc-tests.py -extended

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJun 21 2017, 15:14

The validity check should be in the config setter, I think.

qa/rpc-tests/abc-rpc.py
40 ↗(On Diff #608)

Just use UAHF_START_TIME . If you have something that rely on specific time, your test is going to be flacky anyway.

deadalnix requested changes to this revision.Jun 21 2017, 18:13

Some changes, but nothing critical. I still need to understand the python test better.

src/rpc/abc.cpp
28 ↗(On Diff #608)

I don't think you need to lock cs_main here.

64 ↗(On Diff #608)

Create a block to keep the lock as little as possible.

81 ↗(On Diff #608)

There should be a check in there. That's the inly way to ensure consistency of the config checks across different way to set these configs.

89 ↗(On Diff #608)

This would benefit from being moved after the function on excessive block size.

This revision now requires changes to proceed.Jun 21 2017, 18:13
freetrader edited edge metadata.

Initial batch of fixes from deadalnix review comments

Excludes the mocktime adaptation as that is crucial to
current abc-rpc.py test design for this feature.
An adaptation to that will take some more time.

qa/rpc-tests/test_framework/cdefs.py
32 ↗(On Diff #617)

I added this during initial construction of the abc-rpc.py changes.
Ended up not using it, but since it is a useful default I retained it for the Python framework.

src/rpc/abc.cpp
126 ↗(On Diff #617)

As requested, I added a block to limit the LOCK

28 ↗(On Diff #608)

Agreed, removed

81 ↗(On Diff #608)

In principle I agree that it would be good to move such validation checks into SetUAHFStartTime() .

In practice, this comes with some complications - one needs to pass MTP into the validation method, but cannot do so from the call site at init.cpp:AppInitParameterInteraction() , since that is way too early. So that would require a workaround of introducing a minimum UAHF start time to pass for the non-existent MTP at that point, or have two separate validation methods entirely, one for RPC and one for command line parameter checking.

Last status on Slack discussion was to keep the validation in the RPC call for now.

So I am keeping them here for now.

89 ↗(On Diff #608)

Done - moved to after the excessive block methods

src/test/uahfstarttime_tests.cpp
106 ↗(On Diff #617)

I fixed this call which was using the wrong offset (+ 2h 1s) instead of the +1s that the comment indicates.

qa/rpc-tests/abc-rpc.py
127 ↗(On Diff #617)

The functions above were re-used from abc-p2p-fullblocktest.py as we discussed that factoring out a large-block generation into a common framework module could be done in a subsequent step.

generate_block() below is adapted from next_block() , with added control of the block's nTime so that I can activate the fork easily while also creating controlled block sizes at the same time.

40 ↗(On Diff #608)

I didn't modify this yet because this is more complex / tied in to test design which needs to test some steps before the fork is active, then activate it and do some checks afterwards (e.g. that UAHF start time can no longer be changed after the fork has happened).

Initially I didn't set the time on test node to shortly ahead of fork, and then I ran into various rejection cases where when I mined the "fork pre-activation" blocks, they would get rejected.

So I set the node up to be a few seconds ahead of the fork time now, so that I can mine the blocks and push the MTP to activate comfortably.

This is very stable as it is not using any machine time - the generated blocks also get their nTime set appropriately.

I could try and simplify this test though, but it's going to take me more experimentation.

deadalnix requested changes to this revision.Jun 22 2017, 12:37
deadalnix added inline comments.
qa/rpc-tests/abc-rpc.py
127 ↗(On Diff #617)

These kind of comment belong in the code, not in the review.

137 ↗(On Diff #617)

Use previous time + 1 .

You only need to tweak one block to set it at the HF time and then pile up.

40 ↗(On Diff #608)

The mock time should only influence what block are accepted (2h into the future). Everything else is supposed to be based on MPT. You can mock time into the future, set activation for the fork at the same time, and mine at present time. All the validation should be based on MPT anyway because it is what matters.

This revision now requires changes to proceed.Jun 22 2017, 12:37
qa/rpc-tests/abc-rpc.py
137 ↗(On Diff #617)

This is for the genesis block case.

For the general case in l.145, if no ntime is passed, this is exactly what is done (use previous time +1 for the new block).

You only need to tweak one block to set it at the HF time and then pile up.

Yes, the test should be simplified in regard to how it uses this method. I'm looking into that.

So looking into this, the RPC test is way overcompilcated. It can do away with all the block generation scafolding and just use generate. You just change the mocked time on the node when you want to activate.

src/test/uahfstarttime_tests.cpp
71 ↗(On Diff #617)

You should check for the nothrow that the config has been updated accordingly.

freetrader edited edge metadata.

Simplify test as suggested by review

deadalnix requested changes to this revision.Jun 22 2017, 17:45

Ok a few changes, but it's mostly cosmetics at this stage. I like what this looks like now.

qa/rpc-tests/abc-rpc.py
96 ↗(On Diff #632)

getstart is a function name, not a variable name (it refers to an action).

97 ↗(On Diff #632)

st doesn't look like it is reused, and the name do not make things particularly more explicit, you can fold in next line.

101 ↗(On Diff #632)

You can assign self.nodes[0] to a local variable node, that'ss be simpler.

125 ↗(On Diff #632)

dito x2

150 ↗(On Diff #632)

dito x2 x2

Also, because it is repeated 3 times, I think it make sense to factor this pattern.

src/test/uahfstarttime_tests.cpp
72 ↗(On Diff #632)

You should check for the nothrow that the config has been updated accordingly.

This revision now requires changes to proceed.Jun 22 2017, 17:45
freetrader edited edge metadata.

More cleanups after review

  • abc-rpc.py refactorings
  • check no-throw results in unit test, use Boost checks instead of asserts
qa/rpc-tests/abc-rpc.py
96 ↗(On Diff #632)

Thanks, was bad naming.

97 ↗(On Diff #632)

Fixed

101 ↗(On Diff #632)

Done

125 ↗(On Diff #632)

Refactored

150 ↗(On Diff #632)

Refactored

src/test/uahfstarttime_tests.cpp
72 ↗(On Diff #632)

Fixed, went over the file and did this plus replacing any assert() with Boost checks.

This revision is now accepted and ready to land.Jun 22 2017, 18:51
This revision was automatically updated to reflect the committed changes.