Allow whitelisting outgoing connections #17167
Conversation
3b91c15 to
ad1991a
Compare
806c237 to
bcb43ce
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. |
17bf6da to
dce1080
Compare
|
Concept ACK. As for implementation, I'm not sure about having This doesn't allow having different flags for a IP for incoming and outgoing. Not sure that's ever necessary, but this seems a bit muddled to me. Might also consider e.g. Edit: in a way you already do this by having separate |
|
As you noticed, the implementation already doesn't treat in/out as permissions. Using |
dce1080 to
911d1ac
Compare
911d1ac to
b7463a9
Compare
b7463a9 to
d465ea1
Compare
d465ea1 to
267dc62
Compare
|
Rebased |
267dc62 to
98db866
Compare
Github-Pull: bitcoin#17167 Rebased-From: a2a21f9
…ing connections Github-Pull: bitcoin#17167 Rebased-From: 36cc299
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
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
…ectIncomingTxs 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/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
…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
|
Concept ACK |
|
This would allow us to use transient (aka one-time / random / disposable) I2P addresses when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
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/bitcoin#17167
|
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened. |
|
@luke-jr Are you intending to work on this? |
|
Seems a little weird because I tried to filter bitcoin core pull requests with Whereas, this PR got last 2 Concept ACK or comments agreeing from active contributors in May 2022. And a Concept ACK from present maintainer.
Anyway, PR author has not commented since Jul 7, 2021 so we cannot blame others for this PR to get more review and eventually merged. Whitelist could be a taboo in bitcoin, however this is already possible manually. This PR makes it easier to manage your outgoing connections. I haven't reviewed each commit but I conceptually agree with the idea. |
|
@brunoerg I have never stopped maintaining it. |
|
@luke-jr Cool, just to know, because it's been closed and I am interesting on it. |
|
Concept ACK |
…enum class ConnectionDirection c77de62 net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection (Luke Dashjr) Pull request description: Refactor split out of bitcoin#17167 ACKs for top commit: practicalswift: cr ACK c77de62: patch looks correct & `enum class` is strictly better Tree-SHA512: 40a1bf69d8ab2651b04ba6adbab789369a5a1a29a64ba764c3e6aab575b7943ea8dfd6e35b0abf5bcffa10e7265f4b523a93aa899c0fd581a84fc51ae5377b90
…enum class ConnectionDirection c77de62 net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection (Luke Dashjr) Pull request description: Refactor split out of bitcoin#17167 ACKs for top commit: practicalswift: cr ACK c77de62: patch looks correct & `enum class` is strictly better Tree-SHA512: 40a1bf69d8ab2651b04ba6adbab789369a5a1a29a64ba764c3e6aab575b7943ea8dfd6e35b0abf5bcffa10e7265f4b523a93aa899c0fd581a84fc51ae5377b90
…enum class ConnectionDirection c77de62 net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection (Luke Dashjr) Pull request description: Refactor split out of bitcoin#17167 ACKs for top commit: practicalswift: cr ACK c77de62: patch looks correct & `enum class` is strictly better Tree-SHA512: 40a1bf69d8ab2651b04ba6adbab789369a5a1a29a64ba764c3e6aab575b7943ea8dfd6e35b0abf5bcffa10e7265f4b523a93aa899c0fd581a84fc51ae5377b90
Rethinking/rewrite of #10594 in light of whitelist flags: new flags are added for "in" and "out" to specify the connection direction matched. Both can be specified to match either direction.