[net] Cleanup logic around connection types #19316

amitiuttarwar · 2020-06-17T21:40:23Z

This is part 1 of #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.

DrahtBot · 2020-06-17T23:07:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

[p2p] Add Peer struct for per-peer data in net processing #19607 ([p2p] Add Peer struct for per-peer data in net processing by jnewbery)
refactor: Cleanup thread ctor calls #19064 (refactor: Cleanup thread ctor calls by hebasto)
Periodically update DNS caches for better privacy of non-reachable nodes #18421 (Periodically update DNS caches for better privacy of non-reachable nodes by naumenkogs)
p2p: Try to preserve outbound block-relay-only connections during restart #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)

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 · 2020-06-18T07:18:46Z

Concept ACK.

Is enum ConnectionType the right approach? It worries me that these things are not mutually-exclusive. Like, ADDRFETCH is also OUTBOUND.
It's true that it's non-ambiguous right now, but maybe we want to make them a bitmask in advance for the sake of explicitness, since we are already refactoring it?

jnewbery · 2020-06-18T19:56:11Z

Concept ACK.

Is enum ConnectionType the right approach? It worries me that these things are not mutually-exclusive. Like, ADDRFETCH is also OUTBOUND.

I think enum is the right approach. OUTBOUND here means a connection that is created automatically by the OpenConnections thread. Yes, there are other kinds of connection that the node originates, but for clarity, I don't think those should be referred to as 'OUTBOUND'.

I think we definitely don't want a bitmask of different connection capabilities. That leads to an combinatorial explosion of connection types that you need to either test or explicitly disallow.

amitiuttarwar · 2020-06-18T21:03:27Z

@naumenkogs its a good question to ask, but I think enforcing mutual exclusivity makes more sense and what allows for a lot of the simplicity. @jnewbery explained the main reasons, I want to add some additional points

everything but INBOUND originates from the node, but OUTBOUND refers to a specific type, which is the most common, full-relay, outbound connection. so I do think its exclusive to the other connection types. on the enum I added OUTBOUND, // full relay connections (blocks, addrs, txns), but lmk if you have suggestions on how it could be clarified better.
if we support combinations, we have to add many behavioral checks to ensure logical adherence. lots of the cleanup in this PR shed light on how brittle/annoying it is to have those checks scattered everywhere. so, I think if we are adding connection types in the future, designing the conn_type to fit into the flat structure better supports code-simplicity-over-time as well.

On a different note, I have a question with the fuzz tests & would appreciate any guidance.

Since I change the signature of the CNode constructor, I'm causing a test failure. I found this function to support fuzz testing with enums, but am unclear on how to implement. It seems I need to add kMaxValue to the enum.

I'm wondering if the pattern is:
a) mimic the enum in the tests & add kMaxValue there, or
b) import something into net.cpp and define directly on the code implementation of the enum.
Option B seems strange to add that dependency to net, but option A seems like it would be easy to forget to add an enum type to the fuzz test if we add a type to the cpp code in the future.

@practicalswift, @MarcoFalke, any chance either of you are able to advise? 🙏🏽

practicalswift · 2020-06-22T09:25:11Z

@amitiuttarwar Thanks for the ping! I'd suggest using PickValueInArray like in these examples:

$ git grep -E 'PickValueInArray.*\(\{.+' "src/test/fuzz/**.cpp"
src/test/fuzz/bloom_filter.cpp:        static_cast<unsigned char>(fuzzed_data_provider.PickValueInArray({BLOOM_UPDATE_NONE, BLOOM_UPDATE_ALL, BLOOM_UPDATE_P2PUBKEY_ONLY, BLOOM_UPDATE_MASK}))};
src/test/fuzz/fees.cpp:    const FeeReason fee_reason = fuzzed_data_provider.PickValueInArray({FeeReason::NONE, FeeReason::HALF_ESTIMATE, FeeReason::FULL_ESTIMATE, FeeReason::DOUBLE_ESTIMATE, FeeReason::CONSERVATIVE, FeeReason::MEMPOOL_MIN, FeeReason::PAYTXFEE, FeeReason::FALLBACK, FeeReason::REQUIRED});
src/test/fuzz/kitchen_sink.cpp:    const TransactionError transaction_error = fuzzed_data_provider.PickValueInArray<TransactionError>({TransactionError::OK, TransactionError::MISSING_INPUTS, TransactionError::ALREADY_IN_CHAIN, TransactionError::P2P_DISABLED, TransactionError::MEMPOOL_REJECTED, TransactionError::MEMPOOL_ERROR, TransactionError::INVALID_PSBT, TransactionError::PSBT_MISMATCH, TransactionError::SIGHASH_MISMATCH, TransactionError::MAX_FEE_EXCEEDED});
src/test/fuzz/message.cpp:        (void)SigningResultString(fuzzed_data_provider.PickValueInArray({SigningResult::OK, SigningResult::PRIVATE_KEY_NOT_AVAILABLE, SigningResult::SIGNING_FAILED}));
src/test/fuzz/netaddress.cpp:    const Network network = fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION});
src/test/fuzz/policy_estimator.cpp:        (void)block_policy_estimator.estimateRawFee(fuzzed_data_provider.ConsumeIntegral<int>(), fuzzed_data_provider.ConsumeFloatingPoint<double>(), fuzzed_data_provider.PickValueInArray({FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}), fuzzed_data_provider.ConsumeBool() ? &result : nullptr);
src/test/fuzz/policy_estimator.cpp:        (void)block_policy_estimator.HighestTargetTracked(fuzzed_data_provider.PickValueInArray({FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}));
src/test/fuzz/script_interpreter.cpp:                (void)SignatureHash(script_code, tx_to, in, fuzzed_data_provider.ConsumeIntegral<int>(), ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}), nullptr);
src/test/fuzz/script_interpreter.cpp:                    (void)SignatureHash(script_code, tx_to, in, fuzzed_data_provider.ConsumeIntegral<int>(), ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}), &precomputed_transaction_data);
src/test/fuzz/script_sign.cpp:                (void)signature_creator.CreateSig(provider, vch_sig, address, ConsumeScript(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}));
src/test/fuzz/signature_checker.cpp:    const SigVersion sig_version = fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0});
src/test/fuzz/system.cpp:            const OptionsCategory options_category = fuzzed_data_provider.PickValueInArray<OptionsCategory>({OptionsCategory::OPTIONS, OptionsCategory::CONNECTION, OptionsCategory::WALLET, OptionsCategory::WALLET_DEBUG_TEST, OptionsCategory::ZMQ, OptionsCategory::DEBUG_TEST, OptionsCategory::CHAINPARAMS, OptionsCategory::NODE_RELAY, OptionsCategory::BLOCK_CREATION, OptionsCategory::RPC, OptionsCategory::GUI, OptionsCategory::COMMANDS, OptionsCategory::REGISTER_COMMANDS, OptionsCategory::HIDDEN});

jnewbery

utACK f5f0e07 (once the fuzzer is fixed)

I've left some optional docs suggestions inline and one more here:

In net.cpp L2387: s/Initiate outbound connections from -addnode/Initiated manual connections

Functionally this looks good to me. Very nice cleanup!

src/net.cpp

src/net.h

amitiuttarwar · 2020-06-24T20:30:26Z

fixed fuzz tests, rebased & incorporated review comments

@jnewbery thank you for review 🙌🏽 I took all your suggestions.

@practicalswift thanks for the tip. Tests are now passing. Is there a time the ConsumeEnum() template would make more sense?

@ariard continuing convo from your comment here.

IMO I would adopt a more fine-grained typology. Right now it sounds to confuse type of traffic (block, addr, tx) and type of connection selection (outbound/inbound, normal/anchor, ...)

Can you explain what you are suggesting? I agree that this proposal flattens different aspects of connection behaviors, but I believe this is advantageous for simpler code, as explained above [1] [2].

As a follow-up, I think we may scope [wtxid relay] under a connection type

Considering this possibility. I haven't reviewed #18044 in depth, but I'm not sure it conceptually makes sense. wtxid relay seems more in line with content sent over the connection (like preferences expressed in the version message) than type of connection. I think you could have wtxid relay for connections that are inbound, outbound, and manual connections?

jnewbery · 2020-06-25T15:07:38Z

ACK the final state of this, but the intermediate commits break the fuzz build because the fuzz changes only appear in the final commit. Ideally those changes would be in the same commits as the changes to the CNode ctors so all intermediate commits build.

vasild · 2020-06-25T19:17:18Z

enum class ConnectionType {
    INBOUND, /**< peer initiated connections */
    OUTBOUND, /**< full relay connections (blocks, addrs, txns) made automatically. Addresses selected from AddrMan. */
    MANUAL, /**< connections to addresses added via addnode or the connect command line argument */
    FEELER, /**< short lived connections used to test address validity */
    BLOCK_RELAY, /**< only relay blocks to these automatic outbound connections. Addresses selected from AddrMan. */
    ADDR_FETCH, /**< short lived connections used to solicit addrs */
};

This is somewhat confusing because it mixes unrelated things in one enum - connection initiator (us/outbound, them/inbound) with connection capabilities. Something like having enum Status { valid, invalid, blue }.

I think the following questions have unclear answer, looking at the above enum:

Are INBOUND connections full relay connections (blocks, addrs, txns)?
Are MANUAL connections full relay connections (blocks, addrs, txns)?
Are FEELER connections inbound or outbound, that is - who initiated the connection?
Are ADDR_FETCH connections inbound or outbound, that is - who initiated the connection?

jnewbery · 2020-06-25T19:33:58Z

This is somewhat confusing because it mixes unrelated things in one enum - connection initiator (us/outbound, them/inbound) with connection capabilities. Something like having enum Status { valid, invalid, blue }.

Please see the above discussion about this, particularly #19316 (comment). Another way to explain this is that these different connection types are already logically separate in the code, but that separation is done using multiple bools. However, combining those different bools is invalid - there's no such thing as an inbound feeler connection, or a manual block_relay connection for example. That leads to logic bugs like the one fixed in this PR here: f2e183e. Flattening the internal model of these different connection types should make those bugs less likely.

I think the following questions have unclear answer, looking at the above enum...

That's fair. Perhaps these comments could be expanded to explicitly say who initiates the connection for each type (hint: it's our side for all connection types except inbound).

vasild

Approach ACK

Just some minor suggestions.

Was the bug fixed by commit [net] Fix bug where AddrFetch connections would be counted as outbound full relay present in master before this PR or it was introduced by some of the preceding commits in this PR? If the latter then maybe squash it into the commit that introduced the bug.

Maybe also squash the fixup from [test] Update fuzz test with new CNode function signature into the commit which broke the tests.

src/net.h

src/net_processing.cpp

src/net.cpp

src/net.h

src/net_processing.cpp

src/net.h

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

sipa · 2020-08-14T23:50:10Z

Posthumous ACK 01e2830

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 #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

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 #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/bitcoin#19316 (comment) & bitcoin/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

…e 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

Summary: ``` -BEGIN VERIFY SCRIPT- sed -i 's/a oneshot/an addrfetch/g' src/chainparams.cpp #comment sed -i 's/oneshot/addrfetch/g' src/net.cpp #comment sed -i 's/AddOneShot/AddAddrFetch/g' src/net.h src/net.cpp sed -i 's/cs_vOneShots/m_addr_fetches_mutex/g' src/net.h src/net.cpp sed -i 's/vOneShots/m_addr_fetches/g' src/net.h src/net.cpp sed -i 's/fOneShot/m_addr_fetch/g' src/net.h src/net.cpp src/net_processing.cpp sed -i 's/ProcessOneShot/ProcessAddrFetch/g' src/net.h src/net.cpp -END VERIFY SCRIPT- ``` Partial backport (1/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@3f1b714 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8711

Summary: ``` - extract inbound & outbound types ``` Partial backport (2/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@26304b4 Depends on D8711. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8712

Summary: Partial backport (3/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@1521c47 Depends on D8712. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8713

Summary: Partial backport (4/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@0e52a65 Depends on D8713. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, majcosta Reviewed By: #bitcoin_abc, PiRK, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8714

Summary: Partial backport (5/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@e1bc298 Depends on D8714. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8715

Summary: Partial backport (6/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@af59feb Depends on D8715. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8716

Summary: ``` - AddrFetch connections are short lived connections used to getaddr from a peer - previously called "one shot" connections ``` Partial backport (7/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@442abae Depends on D8716. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8717

Summary: Partial backport (8/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@46578c0 Depends on D8717. Test Plan: ninja Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8718

Summary: ``` - Directly maintaining the connection type prevents having to deduce it from several flags. ``` Partial backport (9/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@d3698b5 Depends on D8718. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8719

Summary: Partial backport (10/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@49efac5 Depends on D8719. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8720

Summary: Partial backport (11/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@1492342 Depends on D8720. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, majcosta Reviewed By: #bitcoin_abc, PiRK, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8721

Summary: Partial backport (12/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@7b322df Depends on D8721. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8722

Summary: Partial backport (13/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@60156f5 Depends on D8722. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8723

…onnections Summary: Partial backport (14/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@4972c21 Depends on D8723. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8724

…d full relay Summary: ``` The desired logic is for us to only open feeler connections after we have hit the max count for outbound full relay connections. A short lived AddrFetch connection (previously called oneshot) could cause ThreadOpenConnections to miscount and mistakenly open a feeler instead of full relay. ``` Partial backport (15/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@35839e9 Depends on D8724. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8725

Summary: ``` Make the connection counts explicit and extract into interface functions around m_conn_type. Using explicit counting and switch statements where possible should help prevent counting bugs in the future. ``` Partial backport (16/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@7f7b83d Depends on D8725. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8726

Summary: ``` Extract logic that check multiple connection types into interface functions & structure as switch statements. This makes it very clear what touch points are for accessing `m_conn_type` & using the switch statements enables the compiler to warn if a new connection type is introduced but not handled for these cases. ``` Partial backport (17/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@2f2e13b Depends on D8726. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8727

Summary: Partial backport (18/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@bc5d65b Depends on D8727. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8728

Summary: Completes backport (19/19) of core [[bitcoin/bitcoin#19316 | PR19316]]: bitcoin/bitcoin@01e2830 Depends on D8728. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8729

amitiuttarwar mentioned this pull request Jun 17, 2020

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

DrahtBot added P2P RPC/REST/ZMQ Tests Validation labels Jun 17, 2020

amitiuttarwar force-pushed the 2020-06-conn-refactor branch from 9fdd50e to 2a8302e Compare June 17, 2020 22:47

amitiuttarwar force-pushed the 2020-06-conn-refactor branch from 2a8302e to f5f0e07 Compare June 17, 2020 23:14

This was referenced Jun 18, 2020

refactor: replace CConnman pointers by references in net_processing.cpp #19174

Merged

Add extra thread for scheduler, move TorControl and OpenAddedConnections to scheduler #18925

Closed

This was referenced Jun 18, 2020

p2p: Unify Send and Receive protocol versions #17785

Merged

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

DrahtBot mentioned this pull request Jun 19, 2020

Re 8000 place automatic bans with discouragement filter #19219

Merged

ariard mentioned this pull request Jun 22, 2020

Use wtxid for transaction relay #18044

Merged

2 tasks

jnewbery reviewed Jun 22, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 23, 2020

net processing: Move orphan reprocessing to a global #19364

Closed

amitiuttarwar force-pushed the 2020-06-conn-refactor branch from f5f0e07 to 43cec7b Compare June 24, 2020 18:49

This was referenced Jun 25, 2020

refactor: Cleanup thread ctor calls #19064

Merged

p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater #14033

Merged

vasild reviewed Jun 26, 2020

View reviewed changes

jonatack mentioned this pull request Jun 29, 2020

rpc, cli: add network in/out connections to getnetworkinfo and -getinfo #19405

Merged

jonatack reviewed Jul 1, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

fanquake merged commit ce3bdd0 into bitcoin:master Aug 12, 2020

jnewbery mentioned this pull request Aug 13, 2020

net: Remove unused conn_type default arg in OpenNetworkConnection #19707

Closed

amitiuttarwar mentioned this pull request Aug 14, 2020

[net] Cleanup connection types- followups #19724

Merged

amitiuttarwar mentioned this pull request Aug 15, 2020

[RPC] Add connection type to getpeerinfo, improve logs #19725

Merged

ariard mentioned this pull request Dec 29, 2020

p2p: Add DISABLETX message for negotiating block-relay-only connections #20726

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[net] Cleanup logic around connection types #19316

[net] Cleanup logic around connection types #19316

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[net] Cleanup logic around connection types #19316

[net] Cleanup logic around connection types #19316

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!