As discussed in #19943, this test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of Bitcoin Core to actually relay transactions to a blocksonly node and benefit from the whitelistforcerelay parameter.
72 | @@ -74,6 +73,10 @@ def run_test(self): 73 | 74 | self.log.info('Check that the tx from forcerelay first_peer is relayed to others (ie.second_peer)') 75 | with self.nodes[0].assert_debug_log(["received getdata"]): 76 | + # NB: in practice, first_peer would never send us transactions since we're a blocksonly node. 77 | + # But if first_peer runs a modified version of Bitcoin Core to bypass relay checks and forward
nit: s/forward/forwards
My thinking was that forward was going in pair with bypass (both infinitive), but I'm not a native speaker, I can definitely apply that.
Yeah first_peer is running a modified version of Bitcoin Core to bypass relay checks. Once it has done that the first_peer forwards the transactions. But as I said I don't think we are fussy on grammar for comments on this project and I wouldn't want to put people like you off from adding comments in future! So feel free to leave it. We'll see if Gleb has more substantial suggestions.
Concept ACK. Thanks for opening this @t-bast.
I think we are pretty lenient on style, grammar etc of comments on this project so feel free to ignore the nit. I personally like linking to the issue number (where the discussion is) too.
Anything that needs adding to this comment @naumenkogs?
Done, let me know if that's better.
55 | @@ -56,33 +56,35 @@ def run_test(self): 56 | self.nodes[0].p2p.wait_for_tx(txid) 57 | assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) 58 | 59 | - self.log.info('Check that txs from forcerelay peers are not rejected and relayed to others') 60 | - self.log.info("Restarting node 0 with forcerelay permission and blocksonly") 61 | - self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"]) 62 | + self.log.info('Check that txs from relay peers are not rejected and relayed to others')
"Forcerelay' clearly meant a peer with FORCERELAY permission. Now that you call it "relay", it's not clear it's about permissions, because we commonly use "relay" in many contexts. I would prefer "RELAY-whitelisted" or something like that hereinafter.
You're right, that was confusing. Fixed in https://github.com/bitcoin/bitcoin/pull/19963/commits/3abe9f866c17fb4fde623d410d59f269b673645f
Still no idea what's "NB" :) Otherwise looks good if you see my new suggestion.
Still no idea what's "NB" :)
Heh, it's "Nota Bene", we probably use that latin abbreviation too often in France :)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
55 | @@ -57,29 +56,35 @@ def run_test(self): 56 | self.nodes[0].p2p.wait_for_tx(txid) 57 | assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) 58 | 59 | - self.log.info('Check that txs from forcerelay peers are not rejected and relayed to others') 60 | - self.log.info("Restarting node 0 with forcerelay permission and blocksonly") 61 | - self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"]) 62 | + self.log.info('Check that txs from RELAY-whitelisted peers are not rejected and relayed to others') 63 | + self.log.info("Restarting node 0 with relay permission and blocksonly") 64 | + self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistrelay", "-blocksonly"])
Can't this be written shorter as:
self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"])
I thought so too, but interestingly it doesn't work.
When setting whitelist=relay@127.0.0.1 and doing a getpeerinfo, the permissions are correctly set to relay, but whitelisted is set to false...
whitelisted is there for backward compatibility only:
// If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility).
Your test should still work even after marco's suggestion? You just have to update the following stuff too: drop the legacy assert_equal(peer_1_info['whitelisted'], True) stuff
(I'm fine keeping it as is too)
Great, thanks for the explanation, the rest of the test indeed works fine once changed. I'll remove that check if that is a deprecated flag, it can be misleading for newcomers.
55 | @@ -57,29 +56,35 @@ def run_test(self): 56 | self.nodes[0].p2p.wait_for_tx(txid) 57 | assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) 58 | 59 | - self.log.info('Check that txs from forcerelay peers are not rejected and relayed to others') 60 | - self.log.info("Restarting node 0 with forcerelay permission and blocksonly") 61 | - self.restart_node(0, ["-persistmempool=0", "-whitelist=127.0.0.1", "-whitelistforcerelay", "-blocksonly"]) 62 | + self.log.info('Check that txs from RELAY-whitelisted peers are not rejected and relayed to others')
self.log.info('Check that txs from peers with relay-permission are not rejected and relayed to others')
Would prefer to use lowercase (identical to the flag name for the whitelist setting)
Done
I agree with Marco even though it was my suggestion to use the uppercase. We definitely use lowercase for permissions, I was just referring to the enum, which has less prio in this case.
8 | @@ -9,7 +9,6 @@ 9 | from test_framework.test_framework import BitcoinTestFramework 10 | from test_framework.util import assert_equal 11 | 12 | -
I think pep-8 wants two lines here
Done
ACK
some style nits, which you are free to ignore
We normally don't commit "fixup fixup" like that, so please squash into 1 commit after you address marco's comments :)
82 | with self.nodes[0].assert_debug_log(["received getdata"]): 83 | + # Note that normally, first_peer would never send us transactions since we're a blocksonly node. 84 | + # By activating blocksonly, we explicitly tell our peers that they should not send us transactions, 85 | + # and Bitcoin Core respects that choice and will not send transactions. 86 | + # But if, for some reason, first_peer decides to relay transactions to us anyway, we should relay them to 87 | + # second_peer since we gave RELAY permission to first_peer.
still uppercase :)
Ready to ack, please squash :)
Done!
As discussed in https://github.com/bitcoin/bitcoin/issues/19943, this
test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of
Bitcoin Core to actually relay transactions to a `blocksonly` node and
benefit from the `whitelistforcerelay` parameter.
ACK e15344889aac50aadee9211ac34e466867d5862b