Page MenuHomePhabricator

[tests] nulldummy cleanup
AbandonedPublic

Authored by dagurval on Feb 7 2019, 14:02.

Details

Reviewers
deadalnix
markblundeberg
Fabien
Group Reviewers
Restricted Project
Summary
  • Alias self.nodes[0] as node. There is only 1 node in this test.
  • Remove block counting, there is no bip9 activation here.
  • Moved policy/consensus tag into log line
  • Replace bunch of members with one LastBlock
  • Use 'make_conform_to_ctor'
Test Plan

CI

Diff Detail

Repository
rABC Bitcoin ABC
Branch
nulldummy-refactor
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4913
Build 7889: Bitcoin ABC Buildbot (legacy)
Build 7888: arc lint + arc unit

Event Timeline

markblundeberg added a subscriber: markblundeberg.

OK, just one more change needed really but indeed I suppose it looks cleaner.

What's the plan for this test after NULLDUMMY consensus activation, I'm curious? Obviously it won't accept that last block anymore ...

test/functional/feature_nulldummy.py
55 ↗(On Diff #7211)

This appears in a lot of tests but seems to be unnecessary, as it runs without. Not sure, should remove?

111 ↗(On Diff #7211)

👍 subtle but indeed make_conform_to_ctor does all this (including tx rehash)

120 ↗(On Diff #7211)

Refers to lastblockhash which doesn't exist anymore.

(this path doesn't get used so test passes despite error)

This revision now requires changes to proceed.Feb 8 2019, 02:46
dagurval marked 3 inline comments as done.

lastblockhash -> lastblock.hash

What's the plan for this test after NULLDUMMY consensus activation, I'm curious? Obviously it won't accept that last block anymore ...

This test will be extended to test the activation. These refactors were originally done as part of D2341.

test/functional/feature_nulldummy.py
55 ↗(On Diff #7211)

This will be needed in the follow up change D2341

120 ↗(On Diff #7211)

Nice catch, I'll keep this dead path because it's used in D2341

Fabien requested changes to this revision.Feb 8 2019, 11:04
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/feature_nulldummy.py
66

Previous coinbase_blocks can be removed if looping directly over node.generate()

78

You can simplify: txid1 = node.sendrawtransaction(ToHex(test1txs[0]), True) (that will require to import ToHex from messages.py)

82

Dito

101

Please note that create_block() builds blocks with nVersion =1

This revision now requires changes to proceed.Feb 8 2019, 11:04

removed unneeded block generation, replace bytes_to_hex_str with ToHex, re-added block version

Fabien requested changes to this revision.Feb 8 2019, 12:13
Fabien added inline comments.
test/functional/feature_nulldummy.py
103 ↗(On Diff #7241)

You can use FromHex() here. That will remove the dependeny to BytesIO :
return FromHex(CTransaction(), signresult['hex'])

This revision now requires changes to proceed.Feb 8 2019, 12:13
test/functional/feature_nulldummy.py
103 ↗(On Diff #7241)

Thanks, but unless there is also some other issues with this diff, I will not do more revisions. This diff has had enough revisions, I'm calling this good enough to land.

deadalnix requested changes to this revision.Feb 8 2019, 14:21

This diff suffer from the classic syndrome of doing several things at once, causing discussion about one of these things to delay progress and all the others and everybody getting frustrated.

Changes such as Alias self.nodes[0] as node. There is only 1 node in this test. are trivial and would be already accepted/landed by now if provided independently. But in the current situation, it is delayed by discussion about LastBlock and introduction of dependencies that are all unrelated to the alias creation. Same goes for make_conform_to_ctor and things like cleaning up imports.

Doing several things at once is by far the number one reason PR to open source projects get stuck, not limited to here. If you want this to make progress, you'll have to accept to update this patch to get all the changes as they should be. Because there are 7 changes bundles in one diff, you should expect this patch to require 7 time more revisions than the average patch. This is not desirable for anyone involved, and lowering standards is not an option.

The test plan is also not appropriate.

test/functional/feature_nulldummy.py
43 ↗(On Diff #7241)

Presumably, LastBlock is a block. A block doesn't have a tip.

This name is kinda strange because, pretty much by definition, there could be only one last block. This doesn't seems to be doing anything related to the block being last.

103 ↗(On Diff #7241)

Numerous tests have been updated to remove that dependency. It's not clear to me why we should go back on that front here.

This diff suffer from the classic syndrome of doing several things at once, causing discussion about one of these things to delay progress and all the others and everybody getting frustrated.

No one is discussing any of the things included in this diff. What's happening is that I'm refusing to make this diff do more things.

dagurval marked an inline comment as done.

Rename class LastBlock -> BlockHeader
Rename method tip -> hash_as_int

jasonbcox added inline comments.
test/functional/feature_nulldummy.py
103 ↗(On Diff #7241)
55 ↗(On Diff #7211)

then why not include it in that change, where it is needed?

test/functional/feature_nulldummy.py
55 ↗(On Diff #7211)

It wasn't added in that patch, why is that even a topic here ?

Fabien requested changes to this revision.Feb 12 2019, 07:42

Could you please rebase on current master ? This will close the discussion regarding ToHex()/FromHex()

This revision now requires changes to proceed.Feb 12 2019, 07:42

According to telegram discussion, this change is controversial and needs to be split into 7 diffs.