utACK 6e43829

Very good! This will make Python linting easier!

I still question this goal, because I think having to (for example) manually add each script opcode as an import when you want to use one in a test is needlessly time-wasting for test writers, but for now I'd like to make the more important observation that merging this will likely cause substantive PRs to need to be rebased (punishment for actually including or updating a test!).

For reference, my earlier comments on this idea: #9876 (comment)

@sdaftuar Do you have any examples of substantive PRs that would need to be rebased after merging this one?

@sdaftuar I have to respectfully disagree. Adding opcodes as you use them makes perfect sense to me. It also gives you an overview of what opcodes are actually being checked, as an added bonus, just by looking at the imports and not the tests themselves (which can be a bit long-winded at times, unfortunately). I also have a hard time believing this PR would cause conflicts in a lot of test PRs, and I am fairly sure most of the test developers agree that * imports are just generally bad.

@MarcoFalke It's no so much about a style but about proper development. Importing * means you lose control over what is going where, just like rampant usage of using namespace xyz in C++. In core proper it can be flat out dangerous and lead to unpredictable behavior with version upgrades, which may not necessarily be the case with functional tests, but it's still a step in the right direction.

@MarcoFalke When mentioning the linting improvements I was specifically thinking about finding unused imports (see below on why that matters), not full PEP-8 compliance :-)

Explicit imports in tests make it easier to get a quick feeling for what a specific test case covers. And perhaps more importantly, by using explicit imports while at the same time removing unused imports it makes it very clear what is not covered by a specific test case (not imported ⇔ not tested).

Strong concept ACK!

@MarcoFalke Let's agree that sorting imports alphabetically daily is a waste of time :-) That kind of PR:s can perhaps be avoided by adding a clear policy in developer-notes.md.

I think this is a good change because import * can make unfamiliar python code really hard to read (you have no idea where identifiers are coming from).

I also think @sdaftuar has a good point about script opcodes, and if there could be a solution for referencing opcodes that doesn't rely on import * before making this change, that would also make this PR smaller and a little easier to review. Maybe the solution could be defining an Op class and replacing OP_RETURN with Op.RETURN, etc. Or just accepting strings like "OP_RETURN".

@ryanofsky There is the

import test_framework.script as script
[...]
        p2sh_program = CScript([script.OP_TRUE])
[...]

approach. A bit verbose perhaps, but better than import *.

@MarcoFalke

I agree it is a step in the right direction, but right now is not a good
time to do global refactors that stall progress of other pulls. I think we
should wait until less refactoring happens in the core files of the test
framework (such as https://github.com/bitcoin/bitcoin/pull/10359/files#diff-
bf6de4c07d2e9114c8f01eea45c6cab3L11) and revisit this pull after the dust
has settled.

I don't see what would be stalled by replacing the import statements with explicit ones, aside from the cases of new imports. I'm perfectly content waiting on #10359 and other PR's that are considered high priority though, but I am also fairly certain @jnewbery (author of #10359) is all for this PR being merged, considering he tried the same thing in #9876.

Needs rebase :-)

Rebased!

Concept ACK, needs rebase

EDIT: Although I think it's fine to have small exceptions like for the example of the opcodes (these examples can be separated to their own files).

@jtimon That might make sense, yeah. (As for rebasing, I will hold off on that until I know we are ready to move forward with this.)

Closing this, this seems to be somewhat controversial, and that's not desirable for something that makes changes all over the test framework and causes lots of necessary rebases of other commits.