refactor: Introduce PeerManagerImpl::RejectIncomingTxs #25156
Conversation
maflcko
changed the title
maflcko
force-pushed
the
2205-block-relay-only-why-tx-inv-
branch
from
fae8cc4 to
fa20bae
Compare
|
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. |
maflcko
force-pushed
the
2205-block-relay-only-why-tx-inv-
branch
from
fa20bae to
faefdbd
Compare
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Concept ACK
Can you copy the rationale to the commit log? It currently doesn't explain why you're introducing the new RejectIncomingTxs() function. I'd also suggest dropping the "refactor" tag.
maflcko
force-pushed
the
2205-block-relay-only-why-tx-inv-
branch
from
faefdbd to
fa2b5fe
Compare
|
Fixed feedback |
|
ACK fa2b5fe0c19035df3030e39ba919077a0cda8ac1 |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Code Review ACK fa2b5fe0c19035df3030e39ba919077a0cda8ac1
test/functional/p2p_blocksonly.py
Outdated
There was a problem hiding this comment.
"with relay permission":
I think that even if block-relay-only connections could have relay permission, they still wouldn't have that here because the last restart disabling -blocksonly mode cleared the [email protected] arg from above.
There was a problem hiding this comment.
| self.log.info("Tests with node in normal mode with block-relay-only connections with relay permission") | |
| self.log.info("Tests with node in normal mode with block-relay-only connection, sending an inv") |
Oh good catch. I changed the test to an inv once I realized that permission flags can't be set, but forgot to update the comment.
Currently there are some confusions in net_processing: * There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature. * Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: bitcoin#17167
maflcko
force-pushed
the
2205-block-relay-only-why-tx-inv-
branch
from
fa2b5fe to
fafddaf
Compare
|
Fixed typo in test log. (Thanks Martin!) Should be trivial to re-ACK with |
|
Code review ACK fafddaf |
| conn.sync_send_with_ping() | ||
| assert(int(txid, 16) not in conn.get_invs()) | ||
|
|
||
| def check_p2p_inv_violation(self, peer): |
There was a problem hiding this comment.
This doesn't need to be done here, but if there is a new function for this anyway, it could also be used in the first part blocksonly_mode_tests to remove some (almost) duplicate code.
…ingTxs fafddaf refactor: Introduce PeerManagerImpl::RejectIncomingTxs (MacroFake) Pull request description: Currently there are some confusions in net_processing: * There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature. * Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: bitcoin#17167 ACKs for top commit: MarcoFalke: Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa2b5fe0c1 fafddaf`. jnewbery: Code review ACK fafddaf mzumsande: ACK fafddaf Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490
6 participants
Currently there are some confusions in net_processing:
-blocksonly modeandblock-relay-only, so adjust all comments to use the same nomenclature.block-relay-onlypeers withrelaypermission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: Allow whitelisting outgoing connections #17167