The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14519 (tests: add utility to easily profile node performance with perf by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LGTM, though we don't need it at the moment, this is a sufficiently small and useful change I don't see any reason to consider not merging it.
utACK 98a1846

(sorry for the post-merge review. I only just saw this)

This isn't really safe and I suggest we revert it. add_nodes was designed to only be called once at the top of the test. The idea is that the number of required nodes is declared up front, and each node can then be stopped/started during the test.

We could relax that restriction if necessary, but I don't think it is necessary. Issues with this implementation:

• _initialize_chain() and _initialize_chain_clean() both use the num_nodes property to create each node's directory with chainstate and wallet at the start of the test. If an extra node is added later with add_nodes() then it won't have this preconfigured directory, which will probably be unexpected and difficult to debug.
• if add_nodes() is called later in the test with the num_nodes argument set to >1, each of the additional new nodes will have the same index (see self.nodes.append(TestNode(numnode,...)

@stevenroose. I don't think you need this for your test in ElementsProject/elements#458. You can declare the num_nodes for your test, override the setup_nodes() method to only start the ones you need at the beginning of the test, then update node.args for the nodes you want to update the arguments for before starting them.

Apologies - I should have commented the add_nodes() method more thoroughly when I added it.

Yeah, without documentation, the name add_nodes suggests that you can "add" nodes with it, implying it should be able to be called more than once.

if add_nodes() is called later in the test with the num_nodes argument set to >1, each of the additional new nodes will have the same index (see self.nodes.append(TestNode(numnode,...)

That is not true, that is exactly the issue that I solved.

So yeah, regarding your first point, I setup the num_nodes variable upfront and then add the nodes one by one, because they are all (4) different.

That is not true, that is exactly the issue that I solved.

Apologies, yes - you're right.

I believe the first point is still a problem.