[net] Cleanup connection types- followups #19724

amitiuttarwar · 2020-08-14T22:23:05Z

This PR addresses outstanding review comments from #19316. It further simplifies net.cpp complexity and adds documentation about design goals about different connection types.

amitiuttarwar · 2020-08-14T22:32:07Z

I'm not 100% convinced about replacing IsAddrRelayPeer() with RelayAddrsWithConn() 9345f9c.

Pros:

It is more direct to check the connection type instead of a (missing) data structure based on the connection type.
Easy to update logic in the future
Less mental overhead, less code touch points to glean intention.

Cons:

We are now checking the same conditional in two places- [1] & [2], and need to manually ensure they do not fall out of sync.

Curious to hear what other reviewers think is preferable.

sipa · 2020-08-14T23:53:06Z

@amitiuttarwar I think it makes sense to make the addr-relay decision depend on the connection type. That should be the most authoritative information about the connection. Sure, it's duplication, but if we accidentally forget to create the necessary data structures, test will fail; if we accidentally create them if they're not needed, at worst we've wasted a bit of memory we're already ok with for other nodes.

src/net.cpp

jnewbery · 2020-08-15T10:16:06Z

src/net_processing.cpp

@@ -2433,9 +2433,8 @@ void ProcessMessage(
        UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
        }

-        if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer())
+        if (pfrom.AdvertiseAddressToConn())


If we're cleaning this up, we should really clean it up and move the connman.MarkAddressGood() out of this if statement, since it's not related to address advertising at all. Having it in the same conditional is a trap for developers to fall into.

(specifically, a developer might think "we don't need to advertise our address to FEELER connections, since we disconnect before we actually send the ADDR or GETADDR messages, so we can remove FEELER from AdvertiseAddressToConn", but doing that breaks our FEELER connection logic, because MarkAddressGood() needs to be called to mark the address as good)

cc @sdaftuar

I was looking at this more and I believe we should leave the MarkAddressGood call in the if-block, though it needs an explanation— the reason (I think) is that we don’t want block-relay connections to leave a trace in the addrman that could be leaked by an attacker sniffing our addr responses.

So this just needs a comment explaining why it’s in here.

Can you explain this a bit? I think you're saying that if we connect out to a peer that we use for block-relay-only we don't want to mark it as good (essentially move it from New to Tried and update the last successful connect time) because then another peer might be able to tell that we've connected to them?

My understanding of how we respond to getaddr messages is that we'll respond with (up to) 1000 records taken at random from all records, regardless of whether they're in New or Tried, so marking a peer as good won't change whether we include it in addr responses.

I don't have a great understanding of the details of how addrman works or how we interact with it -- so possibly the fear was unfounded. It used to be the case that you could trivially download a listening node's addrman though, and it also used to be the case (perhaps only long ago?) that we'd leak information about current connections based on time stamps stored in our addrman, so when I worked on #15759 I tried to leave the addrman state unchanged when making block-relay-only connections.

I'm not sure that I fully succeeded in not leaking information, or that the concern is correct to begin with given how addrman already works. So it's possible that with some analysis we could make improvements here. Maybe not appropriate for this pr though...

I believe there are three ways that a peer's record is updated in addrman when we're connected to them:

when we receive a version message from them, we'll mark their address good. That moves the address from the new table to the tried table, and updates the nLastSuccess/nLastTry/nAttempts fields. We only do this if the peer isn't inbound and isn't block-relay-only. I don't believe this change can easily be detected by a peer sending us getaddr messages.

if a peer self-advertises to us, we'll call AddAddressKnown(), which could update the nTime, which is included in the address record that we send to a peer in response to a getaddr request.

when we disconnect from the peer, we'll update the nTime, as long as the peer was marked as fCurrentlyConnected. That happens when we receive a verack from a peer which is not inbound. This doesn't include feeler peers, since we disconnect from them as soon as we receive a version message from them. The nTime is only updated during disconnection so spies can't find out who we're currently connected to ( Reduce fingerprinting through timestamps in 'addr' messages. #5860 (comment)).

In summary, I think block-relay-only peers should have their address marked good when we connect to them (1). If we don't, then we'll update the nLastTry and nAttempts fields when we attempt to connect to them, but not update the nLastSuccess.

The reason it's important in this PR is that placing MarkAddressGood() behind a conditional if (AdvertiseAddressToConn()) is very confusing for anyone reading the code, and makes it easy to introduce subtle bugs.

I think we're taking the wrong approach in the PR as it is right now. At least that condition should be split in 2 blocks:

I think the current code change here is not useful, but only distracts:
I) The comment Advertise our address is removed for no reason
II) The comment (unrelated to advertising our address) is very confusing. I managed to understand it only after reading this discussion
III) The comment Moves address from New to Tried table in Addrman is not sufficient
IV) The change of IsAddrRelayPeer to IsBlockOnlyConn here is very confusing, the two blocks should be split into 2 very separate blocks according to the following reasoning:

We self-announce to every peer that supports self-announcement AND it's a good idea:
1a) not useful to self-announce to feelers as we won't actually send ADDR to them (i think?)
1b) bad to self-announce to block-relay-only (maybe? wrt privacy)

We MarkAddressGood according to the logic we have

w.r.t block-relay-only peers and AddrMan, I agree it's a separate issue, so i'd prefer to preserve the approach status quo here.

The current PR currently satisfies it, but I'm still unhappy with the code changes in this particular area (see the previous comment).

@naumenkogs Would it be enough to just add some more comments to this block? I think it's fine to leave as one block until we actually need to split it, so I'd propose something like this:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7cd4f7c4cb5..4285e8ba898 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2427,6 +2427,21 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) { + // For outbound peers, we try to relay our address (so that other + // nodes can try to find us more quickly, as we have no guarantee + // that an outbound peer is even aware of how to reach us) and do a + // one-time address fetch (to help populate/update our addrman). If + // we're starting up for the first time, our addrman may be pretty + // empty and no one will know who we are. + // + // We also will update the addrman to record connection success for + // these peers (which include OUTBOUND_FULL_RELAY peers and FEELER + // connections) so that addrman will have an up-to-date notion of + // which peers are online and available. + // + // We skip these operations for BLOCK_RELAY peers to avoid + // potentially leaking information about our BLOCK_RELAY + // connections via the addrman or address relay. if (fListen && !::ChainstateActive().IsInitialBlockDownload()) { CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices()); @@ -2446,8 +2461,8 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); pfrom.fGetAddr = true; - // Moves address from New to Tried table in Addrman - // (unrelated to advertising our address) + // Moves address from New to Tried table in Addrman, resolves + // tried-table collisions, etc. m_connman.MarkAddressGood(pfrom.addr); }

I'd suggest making If we're starting up for the first time, our addrman may be pretty empty and no one will know who we are. less ambiguous:
If we're starting up for the first time, it would improve our peering chances. Otherwise, our addrman may be pretty empty, and also no one will know who we are.

But generally, I think this comment:

makes it much more clear

actually convinces me that it's an improvement over the existent code.

incorporated these comments into the changes,

I agree this should be further improved, but I'm hoping this is sufficient for this PR. @naumenkogs are you comfortable with the current state?

jnewbery · 2020-08-15T10:18:49Z

Strong strong concept ACK.

I think it'd be good to extract everything in the while (!interruptNet) in ThreadOpenConnections() into its own function since deeply nested while/for/if blocks obscure control flow and are very often the sources of bugs. Doing that might be scope creep for this PR though.

jnewbery

ACK everything except the AdvertiseAddressToConn() change. I'd much rather leave the conditional as

if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn())

and add a large comment to above the connman.MarkAddressGood(pfrom.addr); to say that this isn't anything to do with sending an addr or getaddr message.

src/net.h

DrahtBot · 2020-08-20T20:24: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: Improve diversification of new connections #19860 (Improve diversification of new connections: privacy and stability by naumenkogs)
Periodically make block-relay connections and sync headers #19858 (Periodically make block-relay connections and sync headers by sdaftuar)
Refactoring and minor improvement for self-advertisements #19843 (Refactoring and minor improvement for self-advertisements by naumenkogs)
net processing: Move block inventory state to net_processing #19829 ([net processing] Move block inventory state to net_processing by jnewbery)
p2p: Remove fGetAddr flag #19794 (p2p: Remove fGetAddr flag by mzumsande)
net: don't try to relay to the address' originator #19763 (net: don't try to relay to the address' originator by vasild)
[RPC] Add connection type to getpeerinfo, improve logs #19725 ([RPC] Add connection type to getpeerinfo, improve logs by amitiuttarwar)
p2p: Unify Send and Receive protocol versions #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
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)
p2p: Avoid forwarding ADDR messages to SPV nodes #17194 (p2p: Avoid forwarding ADDR messages to SPV nodes by naumenkogs)

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.

amitiuttarwar · 2020-08-21T03:40:29Z

thanks for the feedback! all comments should be addressed.

@sipa
I agree with this:

make the addr-relay decision depend on the connection type. That should be the most authoritative information about the connection.

But unfortunately I don't think this is true:

but if we accidentally forget to create the necessary data structures, test will fail;

I updated RelayAddrsWithConn to always return true, recompiled & ran test_runner.py & unit tests. Nothing failed. My concern would be addressed if it did :) I think with #19315, we should be able to add functional tests that would fail if a discrepancy occurs?

Currently I think the downside is probably acceptable, but want to highlight since I don't think we have any automatic protection in place. (unless I'm missing something?)

@jnewbery, RE:

I think it'd be good to extract everything in the while (!interruptNet) in ThreadOpenConnections() into its own function since deeply nested while/for/if blocks obscure control flow and are very often the sources of bugs. Doing that might be scope creep for this PR though.

I 100% agree. I spent so many hours trying to interpret the exact logic around what types of connections are open, and have gotten very confused by the deeply nested logic flows & nuances of things like where a break / continue that's 4 levels deep will pop you back out to. I've taken a first pass at extracting the outer while (!interruptNet) loop here (yes, there are two levels nested): amitiuttarwar@eecc587. However, it needs to be done with great care and I would need to spend a lot more time / build a lot more confidence with these changes before I'd feel comfortable proposing them. Feel free to take a look or further the branch if you're interested :)

jnewbery · 2020-08-21T10:43:11Z

utACK c99b260

have gotten very confused by the deeply nested logic flows & nuances of things like where a break / continue that's 4 levels deep will pop you back out to.

Yes, deeply nested if/while/switch statements hide bugs. If it's possible to factor them out to clarify control flow, we should aim to do that.

I've taken a first pass at extracting the outer while (!interruptNet) loop here

I've had a very quick skim of that branch and it looks like the right direction to me.

I would need to spend a lot more time / build a lot more confidence with these changes before I'd feel comfortable proposing them. Feel free to take a look or further the branch if you're interested :)

I think you're very well positioned to open this PR if you want to. You've already spent hours getting to grips with the logic here 🙂 . I've already got my quota of refactor PRs open, but I can definitely commit to reviewing a PR if you open it. (Equally, we don't need to fix this all at once. We can always leave this for another time).

src/net_processing.cpp

src/net.cpp

src/net.h

ariard · 2020-08-22T22:57:51Z

src/net.h

+     * open MAX_BLOCK_RELAY_ONLY_CONNECTIONS to help prevent against partition
+     * attacks. By not relaying transactions or addresses, these connections
+     * are harder to detect by a third party, thus helping obfuscate the
+     * network topology. Addresses are selected from AddrMan.


You can mark that addresses for {BLOCK_RELAY,OUTBOUND_FULL_RELAY} are drawn from tried table.

Also block-relay network topology.

are they always? what happens if your tried table isn't populated yet ?

Ah sorry, in fact they are drawn from both new/tried tables at anytime. Surely being confused by SelectTriedCollision in net.cpp path!

src/net.h

src/net_processing.cpp

jonatack · 2020-08-25T13:03:40Z

Concept ACK on these changes. Late to the party but will try to review soon-ish.

amitiuttarwar · 2020-08-27T20:10:18Z

thanks for the review @jnewbery & @ariard ! all review comments are addressed

rebased & made some small changes to docs

-BEGIN VERIFY SCRIPT- sed -i 's/OUTBOUND, /OUTBOUND_FULL_RELAY, /g' src/net.h sed -i 's/ConnectionType::OUTBOUND/ConnectionType::OUTBOUND_FULL_RELAY/g' src/test/net_tests.cpp src/test/fuzz/process_message.cpp src/test/fuzz/process_messages.cpp src/net.cpp src/test/denialofservice_tests.cpp src/net.h src/test/fuzz/net.cpp -END VERIFY SCRIPT-

We previously identified if we relay addresses to the connection by checking for the existence of the m_addr_known data structure. With this commit, we answer this question based on the connection type. IsAddrRelayPeer() checked for the existence of the m_addr_known

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

Previously we deduced it was a block-relay-only based on presence of the m_tx_relay structure. Now we have the ability to identify it directly via a connection type accessor function.

Consolidate the logic to determine connection type into one conditional to clarify how they are chosen.

amitiuttarwar · 2020-09-03T00:43:39Z

thank you for the reviews!

good catch on the silent merge conflict @sdaftuar, I’ve rebased to address. (turns out, there was more than one)

I've addressed all review comments. They were getting pretty web-like, so I've tried my best to resolve and focus on any outstanding conversations.

It looks like all of the remaining review comments are on style/documentation. Perhaps we should aim to merge this and address those in a follow-up?

Most of the changes in this commit are style/documentation. It's not like it's a big logic-changing PR with a lot of ACKs will have to invalidate :)

I'd like to get this PR merged sooner rather than later for a couple reasons:

I’d like to get the renames on master to avoid possibility of more silent merge conflicts
there are now two PRs that build on this (Periodically make block-relay connections and sync headers #19858 & p2p: Improve diversification of new connections #19860). further improvements can be addressed in parallel rather than holding up the whole sequence.
I think its important not to discourage review / lose momentum. @jnewbery has ACKed the tip twice already & I’ve had to invalidate because of needing rebase. with 10 commits deep, it doesn't seem trivial to reACK, so I'd rather use that review energy on smaller isolated changes that can be handled in another PR.

just trying to express that if I do get ACKs, I'm going to try to be cautious about invalidating, so might opt for starting another branch/PR if more non-essential changes are requested.

amitiuttarwar · 2020-09-03T02:24:45Z

there's one failing build, but I'm confused by it. nothing in the logs appears to have failed??
https://travis-ci.org/github/bitcoin/bitcoin/jobs/723629947

RandyMcMillan · 2020-09-03T02:36:00Z

@amitiuttarwar - This PR is based on parent commits that aren't passing - the fail may have nothing to do with your change. I recommend rebasing your commit on a recent passing commit.

naumenkogs · 2020-09-03T07:34:20Z

ACK eb1c5d0

laanwj · 2020-09-03T11:27:09Z

Code review ACK eb1c5d0

ariard · 2020-09-03T22:04:57Z

post-merge Code Review ACK eb1c5d0

…BOUND_FULL_RELAY Summary: ``` sed -i 's/OUTBOUND,/OUTBOUND_FULL_RELAY,/g' src/net.h sed -i 's/ConnectionType::OUTBOUND/ConnectionType::OUTBOUND_FULL_RELAY/g' src/test/net_tests.cpp src/test/fuzz/process_message.cpp src/test/fuzz/process_messages.cpp src/net.cpp src/test/denialofservice_tests.cpp src/net.h src/test/fuzz/net.cpp src/avalanche/test/processor_tests.cpp arc lint ``` This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [1/9] bitcoin/bitcoin@8d6ff46 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9770

…onnection Summary: This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [2/9] bitcoin/bitcoin@a6ab1e8 Depends on D9770 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9771

Summary: We previously identified if we relay addresses to the connection by checking for the existence of the m_addr_known data structure. With this commit, we answer this question based on the connection type. IsAddrRelayPeer() checked for the existence of the m_addr_known This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [3/9] bitcoin/bitcoin@dff16b1 Depends on D9771 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9772

Summary: Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com> This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [4/9] bitcoin/bitcoin@ff6b908 Depends on D9772 Test Plan: Proof-reading new comments Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9773

…et correctly Summary: This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [5/9] bitcoin/bitcoin@da3a0be Depends on D9773 Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9774

…tion. Summary: Previously we deduced it was a block-relay-only based on presence of the m_tx_relay structure. Now we have the ability to identify it directly via a connection type accessor function. This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [6/9] bitcoin/bitcoin@1e563ae Depends on D9774 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta, Fabien Reviewed By: #bitcoin_abc, majcosta, Fabien Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9775

…enConnections Summary: Consolidate the logic to determine connection type into one conditional to clarify how they are chosen. This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [7/9] bitcoin/bitcoin@4829b6f Depends on D9775 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9776

Summary: This is a backport of [[bitcoin/bitcoin#19724 | core#19724]] [8/9] bitcoin/bitcoin@d5a57ce Depends on D9776 Test Plan: Proof reading Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9777

…ng default. Summary: This concludes backport of [[bitcoin/bitcoin#19724 | core#19724]] [9/9] bitcoin/bitcoin@eb1c5d0 Depends on D9777 Test Plan: Proof-reading Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9778

amitiuttarwar changed the title ~~[net] Cleanup connection type- followups~~ [net] Cleanup connection types- followups Aug 14, 2020

amitiuttarwar mentioned this pull request Aug 14, 2020

[net] Cleanup logic around connection types #19316

Merged

DrahtBot added P2P Tests labels Aug 14, 2020

amitiuttarwar force-pushed the 2020-08-conn-refactor-followups branch from 899f3b7 to b672f8d Compare August 14, 2020 23:43

maflcko removed the Tests label Aug 15, 2020

maflcko reviewed Aug 15, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Aug 15, 2020

View reviewed changes

amitiuttarwar mentioned this pull request Aug 19, 2020

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

Merged

jnewbery reviewed Aug 20, 2020

View reviewed changes

src/net.h Outdated 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 Aug 20, 2020

net: don't try to relay to the address' originator #19763

Merged

This was referenced Aug 20, 2020

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Merged

p2p: Avoid forwarding ADDR messages to SPV nodes #17194

Closed

amitiuttarwar force-pushed the 2020-08-conn-refactor-followups branch from b672f8d to c99b260 Compare August 21, 2020 02:09

DrahtBot mentioned this pull request Aug 21, 2020

assumeutxo #15606

Closed

18 tasks

This was referenced Aug 21, 2020

p2p: Unify Send and Receive protocol versions #17785

Merged

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

Merged

ariard reviewed Aug 22, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 24, 2020

amitiuttarwar force-pushed the 2020-08-conn-refactor-followups branch from c99b260 to 4c66cc0 Compare August 27, 2020 19:48

DrahtBot removed the Needs rebase label Aug 27, 2020

DrahtBot mentioned this pull request Aug 28, 2020

p2p: Remove fGetAddr flag #19794

Closed

amitiuttarwar added 2 commits September 2, 2020 13:34

[net] Remove unnecessary default args on OpenNetworkConnection

a6ab1e8

amitiuttarwar force-pushed the 2020-08-conn-refactor-followups branch from 4c66cc0 to f87782a Compare September 2, 2020 23:44

amitiuttarwar and others added 8 commits September 2, 2020 17:18

[doc] Explain address handling logic in process messages

ff6b908

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

[trivial] Small style updates

1d74fc7

[test] Add explicit tests that connection types get set correctly

da3a0be

[refactor] Simplify check for block-relay-only connection.

1e563ae

Previously we deduced it was a block-relay-only based on presence of the m_tx_relay structure. Now we have the ability to identify it directly via a connection type accessor function.

[refactor] Simplify connection type logic in ThreadOpenConnections

4829b6f

Consolidate the logic to determine connection type into one conditional to clarify how they are chosen.

[doc] Describe connection types in more depth.

d5a57ce

[doc] Follow developer notes, add comment about missing default.

eb1c5d0

amitiuttarwar force-pushed the 2020-08-conn-refactor-followups branch from f87782a to eb1c5d0 Compare September 3, 2020 00:18

laanwj merged commit 620ac8c into bitcoin:master Sep 3, 2020

ariard mentioned this pull request Sep 3, 2020

Clarify eviction protection scope of outbound block-relay-only peers #19863

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

[net] Cleanup connection types- followups #19724

[net] Cleanup connection types- followups #19724

amitiuttarwar commented Aug 14, 2020 •

edited

Loading

amitiuttarwar commented Aug 14, 2020 •

edited

Loading

sipa commented Aug 14, 2020

jnewbery Aug 15, 2020

sdaftuar Aug 15, 2020

jnewbery Aug 16, 2020

sdaftuar Aug 16, 2020 •

edited

Loading

jnewbery Aug 18, 2020

naumenkogs Sep 1, 2020

naumenkogs Sep 1, 2020

sdaftuar Sep 1, 2020

naumenkogs Sep 2, 2020

amitiuttarwar Sep 3, 2020

jnewbery commented Aug 15, 2020

jnewbery left a comment

DrahtBot commented Aug 20, 2020 •

edited

Loading

amitiuttarwar commented Aug 21, 2020 •

edited

Loading

jnewbery commented Aug 21, 2020

ariard Aug 22, 2020

amitiuttarwar Aug 27, 2020

ariard Sep 3, 2020

jonatack commented Aug 25, 2020

amitiuttarwar commented Aug 27, 2020

amitiuttarwar commented Sep 3, 2020 •

edited

Loading

amitiuttarwar commented Sep 3, 2020

RandyMcMillan commented Sep 3, 2020

naumenkogs commented Sep 3, 2020

laanwj commented Sep 3, 2020

ariard commented Sep 3, 2020

[net] Cleanup connection types- followups #19724

[net] Cleanup connection types- followups #19724

Conversation

amitiuttarwar commented Aug 14, 2020 • edited Loading

amitiuttarwar commented Aug 14, 2020 • edited Loading

sipa commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdaftuar Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Aug 15, 2020

jnewbery left a comment

Choose a reason for hiding this comment

DrahtBot commented Aug 20, 2020 • edited Loading

Conflicts

amitiuttarwar commented Aug 21, 2020 • edited Loading

jnewbery commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Aug 25, 2020

amitiuttarwar commented Aug 27, 2020

amitiuttarwar commented Sep 3, 2020 • edited Loading

amitiuttarwar commented Sep 3, 2020

RandyMcMillan commented Sep 3, 2020

naumenkogs commented Sep 3, 2020

laanwj commented Sep 3, 2020

ariard commented Sep 3, 2020

amitiuttarwar commented Aug 14, 2020 •

edited

Loading

amitiuttarwar commented Aug 14, 2020 •

edited

Loading

sdaftuar Aug 16, 2020 •

edited

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading

amitiuttarwar commented Aug 21, 2020 •

edited

Loading

amitiuttarwar commented Sep 3, 2020 •

edited

Loading