net: Set relay in version msg to peers with relay permission in -blocksonly mode #26248

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2210-version-relay-🥐 changing 3 files +4 −1

maflcko commented at 2:11 PM on October 4, 2022: member

Seems odd to set the relay permission in -blocksonly mode and also ask the peer not to relay transactions.
net: Set relay in version msg to peers with relay permission dddd1acf58
fanquake added the label P2P on Oct 4, 2022
fanquake requested review from dergoegge on Oct 4, 2022
fanquake requested review from mzumsande on Oct 4, 2022
fanquake requested review from naumenkogs on Oct 4, 2022
maflcko commented at 2:19 PM on October 4, 2022: member

Given that no one ever reported a bug about this, it seems likely that this flag was never used in a client that doesn't violate the P2P protocol
DrahtBot commented at 8:24 PM on October 4, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26247 (refactor: Make m_mempool optional in PeerManager by MarcoFalke)
- #24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
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.
naumenkogs approved
naumenkogs commented at 8:16 AM on October 5, 2022: member

ACK https://github.com/bitcoin/bitcoin/commit/dddd1acf58cb7bf328ce3e74d1dc0e8cbd503247

We will now announce relay-tx in VERSION for those peers for which we set the corresponding permission. Previously, they would have to technically violate this flag in some cases (blocksonly), although we didn't punish them for it. This PR resolves this mismatch.
glozow commented at 2:34 PM on October 9, 2022: member

Concept ACK
in src/net_processing.cpp:5227 in dddd1acf58
```
5223 | @@ -5224,6 +5224,7 @@ bool PeerManagerImpl::RejectIncomingTxs(const CNode& peer) const
5224 |  {
5225 |      // block-relay-only peers may never send txs to us
5226 |      if (peer.IsBlockOnlyConn()) return true;
5227 | +    if (peer.IsFeelerConn()) return true;
```
mzumsande commented at 8:54 PM on October 13, 2022:

This means that we would now also disconnect feeler connections for sending us TX / TX-Invs - but that really shouldn't matter either way because we have disconnected feelers anyway before processing any non-VERSION messages from them.

maflcko commented at 4:00 PM on October 17, 2022:

Correct, with the current code this should only ever have any effect on PushNodeVersion (to retain the previous behaviour).
mzumsande commented at 9:04 PM on October 13, 2022: contributor

ACK dddd1acf58cb7bf328ce3e74d1dc0e8cbd503247

Not a fan of the exception to sometimes accept / relay transactions in -blocksonly mode (It causes a lot of additional code complexity / privacy issues and I wonder if anyone out there actually use it), but this makes NetPermissionFlags::Relay consistent with its intended use.
maflcko commented at 4:05 PM on October 17, 2022: member

Not a fan of the exception

I am not a fan either. I think the use case is that you want to treat an external wallet (connected over P2P with relay permission) to be treated the same as a local Bitcoin Core internal wallet.

I am happy to remove the feature, if reviewers prefer that.
mzumsande commented at 4:47 PM on October 17, 2022: contributor

I am happy to remove the feature, if reviewers prefer that.

Since this feature has been supported for a long time, I think removing it would require a bit of communication and wider disucssion (maybe a ML post to find out if people are using this) - I don't think this should be done here, this seems good to go as is.
dergoegge commented at 5:11 PM on October 17, 2022: member

ACK dddd1acf58cb7bf328ce3e74d1dc0e8cbd503247

Would also be ok with removing the feature.

(I did not consider the relay permission in #22340, not sure if that should be revisited)
maflcko merged this on Oct 21, 2022
maflcko closed this on Oct 21, 2022
maflcko deleted the branch on Oct 21, 2022
sidhujag referenced this in commit 96a99a43e8 on Oct 23, 2022
bitcoin locked this on Oct 21, 2023

Contributors

Review Requested
dergoegge

Labels

P2P

net: Set relay in version msg to peers with relay permission in -blocksonly mode #26248

Conflicts