net: Set relay in version msg to peers with relay permission in -blocksonly mode#26248
Hidden character warning
Conversation
|
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 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
naumenkogs
left a comment
naumenkogs
left a comment
There was a problem hiding this comment.
ACK dddd1ac
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.
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
ACK dddd1ac
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.
| { | ||
| // block-relay-only peers may never send txs to us | ||
| if (peer.IsBlockOnlyConn()) return true; | ||
| if (peer.IsFeelerConn()) return true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Correct, with the current code this should only ever have any effect on PushNodeVersion (to retain the previous behaviour).
I am not a fan either. I think the use case is that you want to treat an external wallet (connected over P2P with 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. |
7 participants
Seems odd to set the
relaypermission in -blocksonly mode and also ask the peer not to relay transactions.