- 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'
Details
Details
- Reviewers
deadalnix markblundeberg Fabien - Group Reviewers
Restricted Project
CI
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- nulldummy-refactor
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4916 Build 7895: Bitcoin ABC Buildbot (legacy) Build 7894: arc lint + arc unit
Event Timeline
Comment Actions
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) |
Comment Actions
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 |
| test/functional/feature_nulldummy.py | ||
|---|---|---|
| 66 ↗ | (On Diff #7238) | Previous coinbase_blocks can be removed if looping directly over node.generate() |
| 78 ↗ | (On Diff #7238) | You can simplify: txid1 = node.sendrawtransaction(ToHex(test1txs[0]), True) (that will require to import ToHex from messages.py) |
| 82 ↗ | (On Diff #7238) | Dito |
| 101 ↗ | (On Diff #7238) | Please note that create_block() builds blocks with nVersion =1 |
Comment Actions
removed unneeded block generation, replace bytes_to_hex_str with ToHex, re-added block version
| test/functional/feature_nulldummy.py | ||
|---|---|---|
| 103 | You can use FromHex() here. That will remove the dependeny to BytesIO : | |
| test/functional/feature_nulldummy.py | ||
|---|---|---|
| 103 | 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. | |
Comment Actions
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 | 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 | Numerous tests have been updated to remove that dependency. It's not clear to me why we should go back on that front here. | |
Comment Actions
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.
| test/functional/feature_nulldummy.py | ||
|---|---|---|
| 55 ↗ | (On Diff #7211) | It wasn't added in that patch, why is that even a topic here ? |
Comment Actions
Could you please rebase on current master ? This will close the discussion regarding ToHex()/FromHex()
Comment Actions
According to telegram discussion, this change is controversial and needs to be split into 7 diffs.