refactor: Introduce PeerManagerImpl::RejectIncomingTxs #25156
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
This was referenced May 19, 2022
fa20bae to
faefdbd
Compare
jnewbery
reviewed
May 23, 2022
faefdbd to
fa2b5fe
Compare
|
Fixed feedback |
|
ACK fa2b5fe |
6 tasks
mzumsande
reviewed
Jun 13, 2022
test/functional/p2p_blocksonly.py
Outdated
| @@ -89,6 +89,11 @@ def blocks_relay_conn_tests(self): | |||
| assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False) | |||
| _, txid, _, tx_hex = self.check_p2p_tx_violation() | |||
|
|
|||
| self.log.info("Tests with node in normal mode with block-relay-only connections with relay permission") | |||
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 -whitelist=relay@127.0.0.1 arg from above.
There was a problem hiding this comment.
Suggested change
| 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
fa2b5fe to
fafddaf
Compare
|
Fixed typo in test log. (Thanks Martin!) Should be trivial to re-ACK with |
|
Code review ACK fafddaf |
mzumsande
reviewed
Jun 15, 2022
| @@ -100,6 +105,13 @@ def blocks_relay_conn_tests(self): | |||
| 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.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 15, 2022
…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 fa2b5fe fafddaf`. jnewbery: Code review ACK fafddaf mzumsande: ACK fafddaf Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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