-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: Merge bitcoin#19316, (Partial) 19725, 19724 #5771
Commits on Jan 9, 2024
-
Merge bitcoin#19316: [net] Cleanup logic around connection types
01e2830 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar) bc5d65b [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar) 2f2e13b [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar) 7f7b83d [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar) 35839e9 [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar) 4972c21 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar) 60156f5 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar) 7b322df [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar) 1492342 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar) 49efac5 [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar) d3698b5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar) 46578c0 [doc] Describe different connection types (Amiti Uttarwar) 442abae [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar) af59feb [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar) e1bc298 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar) 0e52a65 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar) 1521c47 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar) 26304b4 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar) 3f1b714 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar) Pull request description: **This is part 1 of bitcoin#19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality. **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions. This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types. (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.) Overview of this PR: * rename `oneshot` to `addrfetch` * introduce `ConnectionType` enum * one by one, add different connection types to the enum * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts) * fix the bug in counting different type of connections * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive. ACKs for top commit: jnewbery: utACK 01e2830 laanwj: Code review ACK 01e2830, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall vasild: ACK 01e2830 sdaftuar: ACK 01e2830. fanquake: ACK 01e2830 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now. jb55: wow this code was messy before... ACK 01e2830 Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
Configuration menu - View commit details
-
Copy full SHA for 2865a2d - Browse repository at this point
Copy the full SHA 2865a2dView commit details -
(Partial) Merge bitcoin#19725: [RPC] Add connection type to getpeerin…
…fo, improve logs a512925 [doc] Release notes (Amiti Uttarwar) 50f94b3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar) df091b9 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar) 395acfa [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar) 49c10a9 [log] Add connection type to log statement (Amiti Uttarwar) Pull request description: After bitcoin#19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field. This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string. Suggested by sdaftuar- bitcoin#19316 (comment) & bitcoin#19316 (comment) ACKs for top commit: jnewbery: Code review ACK a512925. sipa: utACK a512925 guggero: Tested and code review ACK a512925. MarcoFalke: cr ACK a512925 🌇 promag: Code review ACK a512925. Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
Configuration menu - View commit details
-
Copy full SHA for 4143e35 - Browse repository at this point
Copy the full SHA 4143e35View commit details -
Merge bitcoin#19724: [net] Cleanup connection types- followups
eb1c5d0 [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar) d5a57ce [doc] Describe connection types in more depth. (Amiti Uttarwar) 4829b6f [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar) 1e563ae [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar) da3a0be [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar) 1d74fc7 [trivial] Small style updates (Amiti Uttarwar) ff6b908 [doc] Explain address handling logic in process messages (Amiti Uttarwar) dff16b1 [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar) a6ab1e8 [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar) 8d6ff46 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar) Pull request description: This PR addresses outstanding review comments from bitcoin#19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types. ACKs for top commit: naumenkogs: ACK eb1c5d0 laanwj: Code review ACK eb1c5d0 Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5
Configuration menu - View commit details
-
Copy full SHA for 796353a - Browse repository at this point
Copy the full SHA 796353aView commit details -
fix: follow-up missing changes from Merge bitcoin#20188: tests: Add f…
…uzzing harness for CConnman
Configuration menu - View commit details
-
Copy full SHA for aa74d0b - Browse repository at this point
Copy the full SHA aa74d0bView commit details