p2p: Rename fBlocksOnly, Add test #22955

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2109-testInvBlocksonly changing 3 files +25 −15

MarcoFalke commented at 10:16 AM on September 12, 2021: member
fBlocksOnly has several issues:
- The name is confusing
- It is untested
Fix both.
test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method fac66d0a39
fanquake added the label P2P on Sep 12, 2021
MarcoFalke commented at 10:22 AM on September 12, 2021: member

Fun fact: This is covered by the fuzz tests, but obviously they don't check the logic level of the code. https://marcofalke.github.io/btc_cov/fuzz.coverage/src/net_processing.cpp.gcov.html

Functional coverage: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html
MarcoFalke commented at 10:29 AM on September 12, 2021: member

There are some other issues with this code, which will be fixed in follow-ups, because they change the P2P protocol.

p2p: Rename fBlocksOnly, Add test

The new name describes better what the bool does and also limits the confusion of the three different concepts:
* fBlocksOnly (This bool to skip tx invs)
* -blocksonly (A setting to ignore incoming txs)
* block-relay-only (A connection type in the block-relay-only P2P graph)

fa66a7d732

MarcoFalke force-pushed on Sep 12, 2021
DrahtBot commented at 3:25 PM on September 12, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22850 (net: Make m_mempool optional in PeerManager by sriramdvt)
- #21954 (Some minor code changes to improve efficiency of processing TXs by rebroad)
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.
in src/net_processing.cpp:2914 in fa66a7d732
```
2908 | @@ -2909,13 +2909,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2909 |              return;
2910 |          }
2911 |  
2912 | -        // We won't accept tx inv's if we're in blocks-only mode, or this is a
2913 | +        // Reject tx INVs when the -blocksonly setting is enabled, or this is a
2914 |          // block-relay-only peer
2915 | -        bool fBlocksOnly = m_ignore_incoming_txs || (pfrom.m_tx_relay == nullptr);
2916 | +        bool reject_tx_invs{m_ignore_incoming_txs || (pfrom.m_tx_relay == nullptr)};
```
naumenkogs commented at 6:32 AM on September 13, 2021:

Why not calling it ignore? Reject sounds like something proactive.

MarcoFalke commented at 6:39 AM on September 13, 2021:

It will disconnect the peer, which seems proactive and stronger than "ignore"

naumenkogs commented at 6:46 AM on September 13, 2021:

Good point, I forgot it disconnects. Still unsure "reject" captures the meaning tho. I don't have a better alternative to suggest at this point so feel free to ignore.

MarcoFalke commented at 6:56 AM on September 13, 2021:

Happy to change when there is a better name. Resolving for now.
vincenzopalazzo approved
vincenzopalazzo commented at 11:14 PM on September 13, 2021: none

ack https://github.com/bitcoin/bitcoin/pull/22955/commits/fac66d0a39cb0b4bc565b57087ba84dd932e9b6d
laanwj commented at 2:37 PM on September 16, 2021: member

Code review ACK fa66a7d732f9fd60d72f22087f0d5aadf3064bfb
laanwj merged this on Sep 16, 2021
laanwj closed this on Sep 16, 2021
MarcoFalke deleted the branch on Sep 16, 2021
sidhujag referenced this in commit de79e58c0d on Sep 19, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#26886 test: add `rescan utxos` inside MiniWallet's initialization

p2p: Rename fBlocksOnly, Add test #22955

Conflicts