I think instead of mv you'll need to use git mv?

I think instead of mv you'll need to use git mv?

Thanks Marco. Should be fixed now.

concept ACK! I still have to stop and think to distinguish testnode from mininode. I was skeptical about the rename to p2p, but looking at the code has convinced me it makes sense.

Concept ACK

On behalf of future generations of Bitcoin Core contributors: thank you! :)

Concept ACK! Thank you for the PR, @jnewbery.

As a newcomer, I have started to think about mininode as "mocknode" and testnode as just "node". Since the entire network is peer-to-peer anyway, would it be valuable to avoid using that to mean mock?

As a newcomer, I have started to think about mininode as "mocknode" and testnode as just "node".

Yes indeed. A P2PInterface object can be thought of as a mock node.

Since the entire network is peer-to-peer anyway, would it be valuable to avoid using that to mean mock?

In the abstract, it doesn't make sense to talk about a P2PInterface as a peer. When a bitcoind node creates a peering connection to a P2PInterface, it makes sense to call it the node's peer.

Concept ACK

I'm not sure if this makes things less confusing:

In a test where our node has, in current terminology, one connection to a mininode and one to another bitcoind instance, it would in future terminology have one test framework p2p connection and another connection to a regular node, which we probably shouldn't call test framework p2p connection, even though the node would be managed by the test framework (TestNode) as well - this sounds a bit confusing too.

Since I am used to thinking of a mininode as an object first, it would help me if someone could explain where exactly the confusion with mininode was, and why a renaming to p2p or test framework p2p connection is suggested (instead of something more similar like mocknode as suggested by @dhruv).

(Edited for some inaccuracies pointed out by sipa)

The main thing is that people often call mininodes "nodes" when they are not, so we want to remove that naming. I think the main thing is that we don't want people to confuse mininodes (a minimal p2p interface) with testnodes (the bitcoind node we are testing in functional tests). I was skeptical about calling it "p2p" at first as well but, right now, it sounds most sensible to me. Open to discussion.

Calling it MockNode would be even more confusing I think, because that sounds so similar to TestNode, and we often "mock" things (e.g. time) so that we can manipulate them in the functional tests. I'd even be ok with renaming TestNode to MockNode.

I can't really think of anything that mininodes do other than send/receive p2p messages. I think of mininodes as fake peers (but only when connected, as jnewbery said above) that we have full control over, and can use to elicit some behavior from the TestNode(s). But all they can do is p2p messages, nothing more, so it's incorrect to call them "__nodes". The reason I think "p2p" is an appropriate name is that they only implement that portion of node functionality. Mininodes cannot validate anything and cannot make transactions or blocks (only send them). I'm pretty sure they're only able to connect to 1 TestNode at a time. They also don't even always follow p2p protocol (which can be a conservative definition of a node) e.g. p2p_leak.py.

I don't have a strong opinion either way on the renaming, but wanted to respond to this::

But all they can do is p2p messages, nothing more, so it's incorrect to call them "__nodes".

To me, that's exactly what makes something a node: whether or not it interacts with other nodes via the P2P protocol, making it a node of the P2P network graph. Here we just have a very minimal node that doesn't do anything more than that.

@sipa so "peer" == "node"?

A peer is a node that's not yourself (and you're communicating with).

A peer is a node that's not yourself.

This comment pinpoints one source of my confusion: BitcoinTestFramework.num_nodes can set up multiple TestNode instances which are also peers and speak the P2P protocol. This then starts to imply that there are nodes in the P2P network, that are not "peers" as we use that word in the test framework.

Overall, I understand the distinction now and don't have a strong opinion either way. Just my 2c as a rookie.

@gzhao408

There's almost always a better option than the test_framework.util wait_until, which does not grab the lock automatically, and doesn't have any context from the TestFramework instance.

I think this is done in #19752.

Generally I think it's always better to either use the connection returned by add_p2p_connection or index into the p2ps list.

I have no problem with that happening, but it should be in a different PR.

My PR description has made this conversation a bit more abstract than it needs to be. To recenter on the changes this PR is making:

• we have a file that contains our python implementation of a p2p stack. It contains a P2PConnection object (a low level connection to a bitcoind node's P2P interface) and a P2PInterface (a higher level object that allows us to communicate with the node's net_processing layer and register callbacks). This file is currently called mininode.py, which doesn't convey much meaning. A better name for it is p2p.py which more clearly communicates what it is and does.
• data which can be accessed by both the test logic thread and the networking thread is guarded by a global mutex called mininode_lock. Again, that doesn't convey much meaning so rename it to p2p_lock (since it's data that is used by our p2p interface objects).
• fix up related comments.

That's all that this PR does.

Rebased on master.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• test/refactor: reference p2p objects explicitly and remove confusing Test_Node.p2p property #19804 (test: remove confusing p2p property by gzhao408)
• test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) #19801 (test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) by theStack)
• test: Update wait_until usage in tests not to use the one from utils #19752 (test: Update wait_until usage in tests not to use the one from utils by slmtpz)
• [tests] Allow outbound & block-relay-only connections in functional tests.  #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)

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.

rebased

changes look good, but I think there are two more uses of the word "mininode" in comments that would be nice to update

• example_test.py L170
• feature_block.py L56

there are two more uses of the word "mininode" in comments that would be nice to update

Thanks! Not sure how I missed those. I've now removed them.

ACK d5800da

ACK d5800da 🚞

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK d5800da5199527a366024bc80cad7fcca17d5c4a 🚞
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiZNAwAtJbH7PHEONAA5ZbYzkGjBBwJg2v6IwplI/7lrqAlWVZdGOWX3UVZemck
y4TmNTiE4fS9PFqCPtiUgZWS61ZiATQBKG03Kma1Qelb0pvEw1JbRCdKL6NloyYl
us4dHbcPVRWRL9QzMUJg8wbRa0A4wiH5IeI6wpcE3NKKJjefFLnSOqM3nOol3Uix
h+91LJTw5SIwXNYqlejDDtuwH4+6epvPXzujlg64sTSDgN61DpLDsPjWQmMbBhkd
qZ6gGUzcUJWF7n6tGXdOMQ8bXn8jZrGl2fNOirDgbSlNg9yqp2BIwYPD02j5zoBN
Aw36dLz4cxcZFoXmbtP3Jnx6X3qOScbsJaoNw47UzYNe0R58qV2o6fmEsWNU4gTJ
D+bEiltvp4AfiZc0pAnjE0BByeT+Kx79LGgHbb9rya4njKU1aDiLxmhomTCOntqk
c5TfRYHLEEN/aqtf31vHiw8T6HIDGCLJ0WC5Ge81HXIR2ztv1rNafIPuyZRHx3iP
gDMbKONs
=lPcH
-----END PGP SIGNATURE-----

@jnewbery let me know if you want this merged or want to address the nits

I think this is rfm

If you think it's rfm, let's merge it now. I can fix the style comments in a follow-up.