search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Set epochLength=200 in feature_assumevalid.py #1061

Closed kostyantyn closed 5 years ago

kostyantyn commented 5 years ago

This commit is an alternative to https://github.com/dtr-org/unit-e/pull/1056

The motivation for this change is to speed up the test, as by increasing the epoch length, we reduce the number of snapshots the node generates.

I am still in favor of keeping snapshot generation enabled by default to ensure we test that component in every scenario possible.

Signed-off-by: Kostiantyn Stepaniuk kostia@thirdhash.com

kostyantyn commented 5 years ago

Re-worked to have epochLength=500. In this case, we have 5 epochs and we still test all components we have in the system but it significantly decreases the execution time, almost like running without the snapshot creation.

2019-05-03T15:35:01.490000Z TestFramework (INFO): Initializing test directory /var/folders/r9/s635cnq5265dp3bgh1kcw7b40000gn/T/testr_mujl80
2019-05-03T15:35:01.490000Z TestFramework (INFO): Debug file at /var/folders/r9/s635cnq5265dp3bgh1kcw7b40000gn/T/testr_mujl80/node0/regtest/debug.log
Starting node 0 with args: -esperanzaconfig={"epochLength": 500}
Starting node 1 with args: -esperanzaconfig={"epochLength": 500}
Starting node 2 with args: -esperanzaconfig={"epochLength": 500}
2019-05-03T15:35:06.104000Z TestFramework (INFO): Create the first block with a coinbase output to our key
2019-05-03T15:35:06.113000Z TestFramework (INFO): Bury the block 100 deep so the coinbase output is spendable
2019-05-03T15:35:06.481000Z TestFramework (INFO): Create a transaction spending the coinbase output with an invalid (null) signature
2019-05-03T15:35:06.486000Z TestFramework (INFO): Bury the assumed valid block 2100 deep
2019-05-03T15:35:14.424000Z TestFramework (INFO): Start node1 and node2 with assumevalid so they accept a block with a bad signature.
2019-05-03T15:35:15.749000Z TestFramework (INFO): Send blocks to node0. Block 103 will be rejected.
2019-05-03T15:35:19.122000Z TestFramework (INFO): Send all blocks to node1. All blocks will be accepted.
2019-05-03T15:35:34.838000Z TestFramework (INFO): Send blocks to node2. Block 102 will be rejected.
2019-05-03T15:35:35.073000Z TestFramework.mininode (WARNING): Connection lost to 127.0.0.1:13364 due to [Errno 32] Broken pipe
2019-05-03T15:35:35.383000Z TestFramework (INFO): Stopping nodes
2019-05-03T15:35:36.076000Z TestFramework (INFO): Cleaning up /var/folders/r9/s635cnq5265dp3bgh1kcw7b40000gn/T/testr_mujl80 on exit
2019-05-03T15:35:36.077000Z TestFramework (INFO): Tests successful
frolosofsky commented 5 years ago

utACK bc5a294a46259f051b436e57d884201f927864d9

nit: Attached log is not informative enough, I'd better explain result in terms of how long test run at the moment versus proposed change.

We keep snapshot generation by default in all tests to ensure that this feature works correctly

Honestly, I don't understand how snapshot creation is tested in tests which don't use fast sync (i.e. in most of them). I'd disable snapshot creation by default in regtest mode. What do you think?

kostyantyn commented 5 years ago

@frolosofsky snapshot creation is an independent thread which generates the snapshot X epochs. It's independent of the fast sync and this snapshot is needed to support nodes that want to quickly join the network.

I personally prefer to keep this process enabled as it ensures that it's working/not broken as we test it "unintentionally" with every feature. It shouldn't affect the logic of the node (only slows down it a bit)

edited: changed title and the description

AM5800 commented 5 years ago

On the other hand, I am now not sure that 500 is a good value. For me it is not obvious that with this value a snapshot would be created at all. I know that @kostyantyn checked this. But I think it can happen that after we change something inside - snapshot won't be generated anymore and we kind of degrade to disabled snapshot in this test. So maybe we can set a bit smaller value?

kostyantyn commented 5 years ago

@AM5800 on mainnet we have epochLength=50 and snapshot interval=150 epochs. In regtest we have epochLength=5 and snapshot interval=1. I see that in this test we actually try to test the mainnet setup, 2 weeks of work to activate assumevalid. We could configure this test in the same way as mainnet but currently we don't have a configuration parameter for snapshot interval, so we can go with epochLength=200 which will be the closest to mainnet setup. WDYS?

kostyantyn commented 5 years ago

@frolosofsky had an alternative idea to completely disable snapshot creation in the regtest. Will wait with the merge of this PR until we collect enough feedbacks.

AM5800 commented 5 years ago

@kostyantyn If the time difference between 500 and 200 is not drastic(this is what this PR is about) - I would say 200 is better

kostyantyn commented 5 years ago

@AM5800 the difference is 2sec, I changed epochLength to 200

frolosofsky commented 5 years ago

@frolosofsky had an alternative idea to completely disable snapshot creation in the regtest. Will wait with the merge of this PR until we collect enough feedbacks.

There is no need to block your PR. I will open next PR which disables snapshot creation by default in regtest, and we discuss that approach there. Your PR fixes specific problem and I guess it is good to go.

scravy commented 5 years ago

Bitcoin has increased the timeout. And our code can not be faster than bitcoin, only slower(because of additional features like finalization, snapshot etc). So sooner or later we will increase timeout anyway.

...no proof of work?!

AM5800 commented 5 years ago

...no proof of work?!

@scravy but we are talking here about functional tests. They don't mine blocks

scravy commented 5 years ago

They do. That's why you got a nMaxTries variable in the generate call. Just the difficulty is ridiculuously low.

AM5800 commented 5 years ago

@scravy thanks, didn't know this