net: have CConnman handle message sending #8708

theuni commented at 2:00 AM on September 13, 2016: member

See individual commits for more description. In particular, 75ead75890b9b5ca5565a31d5fe9ad31dc9a96fa.

This (along with #8707) fixes the current maxupload test failure.

jonasschnelli added the label P2P on Sep 13, 2016

laanwj commented at 1:40 PM on September 20, 2016: member

Concept ACK

laanwj commented at 11:40 AM on September 25, 2016: member

Needs rebase

jtimon commented at 4:58 PM on September 30, 2016: contributor

Concept ACK

theuni force-pushed on Sep 30, 2016

theuni force-pushed on Oct 4, 2016

in src/main.cpp:None in c295afaf9e outdated

5037 | @@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5038 |  
5039 |          // Be shy and don't send version until we hear
5040 |          if (pfrom->fInbound)
5041 | -            pfrom->PushVersion();
5042 | +            connman.PushVersion(pfrom, GetAdjustedTime());

TheBlueMatt commented at 4:54 PM on October 6, 2016:

Any idea why we use a different time for inbound vs outbound peers? If anything I'd think we'd announce our real time to inbound peers and our adjusted time to outbound ones, but we do the opposite.

sipa commented at 1:16 AM on November 3, 2016:

@TheBlueMatt I'm not sure I see the reasoning for either. I'd expect us to use our best guess of time everywhere.

in src/net.cpp:None in c295afaf9e outdated

 388 | @@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 389 |  
 390 |          // Add node
 391 |          CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest : "", false);
 392 | +
 393 | +        PushVersion(pnode, GetTime());

TheBlueMatt commented at 5:00 PM on October 6, 2016:

Can we go ahead and move the PushVersion logic into InitializeNode? I think thats the right place since it will move into net_processing.cpp with the rest of the "node-message-processing" stuff.

TheBlueMatt commented at 5:06 PM on October 6, 2016: member

Got halfway through a review and realized I just dont know how I feel about this...CNode moving towards a "I speak to the CConnman and serialize messages for the wire" seems like a sane design (and seems to be what you're going for here), but it means there is just no good place to put serialization-version logic that isnt a complete layer violation.

theuni commented at 7:25 PM on October 6, 2016: member

On Oct 6, 2016 1:06 PM, "Matt Corallo" notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

Got halfway through a review and realized I just dont know how I feel about this...CNode moving towards a "I speak to the CConnman and serialize messages for the wire" seems like a sane design (and seems to be what you're going for here), but it means there is just no good place to put serialization-version logic that isnt a complete layer violation.

Yea, I agree. I started creating a CSendMessage that would hold the serialized result, which could go directly to CConnman, but decided for the more straightforward change here. I'm happy to make that change, either as a part of this pr or after.

That has the benefit of testing the serialized result for p2p without actually requiring the send.

In src/main.cpp:

@@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // Be shy and don't send version until we hear if (pfrom->fInbound) - pfrom->PushVersion(); + connman.PushVersion(pfrom, GetAdjustedTime());

Any idea why we use a different time for inbound vs outbound peers? If anything I'd think we'd announce our real time to inbound peers and our adjusted time to outbound ones, but we do the opposite.

Unsure of the reason there.

In src/net.cpp:

@@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char pszDest, bool fCo // Add node CNode pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest : "", false); + + PushVersion(pnode, GetTime());

Can we go ahead and move the PushVersion logic into InitializeNode? I think thats the right place since it will move into net_processing.cpp with the rest of the "node-message-processing" stuff.

I had planned on doing exactly that as a next step, but it looked kinda out of place by itself. With your changes, it makes perfect sense. Happy to change it to whatever makes it easier for you to move.

Glad to see we're in sync :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

TheBlueMatt commented at 7:47 PM on October 6, 2016: member

Yea, seems like we're mostly on the same page here... Probably reasonable to give the serialization to net_processing, though I am concerned with how to merge type with P2P encryption, given that it also changes the message hashing, but only after messages are exchanged (I believe? Maybe @jonasschnelli wants to comment). Even more unsure of how to handle that.

As for this PR, I'm fine with it as it is now, noting that we want to tweak things further as we split code, though I'd prefer we go ahead and move the version message handling to Initialize so we don't end up moving the same block three times.

On October 6, 2016 3:25:54 PM EDT, Cory Fields notifications@github.com wrote:

On Oct 6, 2016 1:06 PM, "Matt Corallo" notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

Got halfway through a review and realized I just dont know how I feel about this...CNode moving towards a "I speak to the CConnman and serialize messages for the wire" seems like a sane design (and seems to be what you're going for here), but it means there is just no good place to put serialization-version logic that isnt a complete layer violation.

Yea, I agree. I started creating a CSendMessage that would hold the serialized result, which could go directly to CConnman, but decided for the more straightforward change here. I'm happy to make that change, either as a part of this pr or after.

That has the benefit of testing the serialized result for p2p without actually requiring the send.

In src/main.cpp:

@@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // Be shy and don't send version until we hear if (pfrom->fInbound) - pfrom->PushVersion(); + connman.PushVersion(pfrom, GetAdjustedTime());

Any idea why we use a different time for inbound vs outbound peers? If anything I'd think we'd announce our real time to inbound peers and our adjusted time to outbound ones, but we do the opposite.

Unsure of the reason there.

In src/net.cpp:

@@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char pszDest, bool fCo // Add node CNode pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest : "", false); + + PushVersion(pnode, GetTime());

Can we go ahead and move the PushVersion logic into InitializeNode? I think thats the right place since it will move into net_processing.cpp with the rest of the "node-message-processing" stuff.

I had planned on doing exactly that as a next step, but it looked kinda out of place by itself. With your changes, it makes perfect sense. Happy to change it to whatever makes it easier for you to move.

Glad to see we're in sync :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #8708 (comment)

laanwj commented at 10:44 AM on October 25, 2016: member

Needs rebase

theuni force-pushed on Oct 27, 2016

theuni commented at 6:47 PM on October 27, 2016: member

rebased. @TheBlueMatt I added some commits to handle the version push in InitializeNode. I left the serialization happening in CConnman for now. Agreed that it's a layer violation and have plans for separating it, but I'd prefer not to change the scope here too much unless you hugely object.

in src/net.cpp:None in f14fc2e48a outdated

2567 |  {
2568 | -    ENTER_CRITICAL_SECTION(cs_vSend);
2569 | -    assert(ssSend.size() == 0);
2570 | -    ssSend << CMessageHeader(Params().MessageStart(), pszCommand, 0);
2571 | -    LogPrint("net", "sending: %s ", SanitizeString(pszCommand));
2572 | +    return {SER_NETWORK, nVersion ? nVersion | flags : pnode->GetSendVersion() | flags, CMessageHeader(Params().MessageStart(), sCommand.c_str(), 0) };

TheBlueMatt commented at 3:14 PM on October 28, 2016:

GetSendVersion needs a lock (or make it atomic with release/aquire).

theuni commented at 4:21 PM on October 28, 2016:

See https://github.com/bitcoin/bitcoin/pull/8708/files#diff-1a8b9d1ad0a6fda5e751286c73102fc2R755

in src/net.cpp:None in f14fc2e48a outdated

2581 | +    // Set the size
2582 | +    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2583 | +    WriteLE32((uint8_t*)&strm[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize);
2584 | +    // Set the checksum
2585 | +    uint256 hash = Hash(strm.begin() + CMessageHeader::HEADER_SIZE, strm.end());
2586 | +    assert(strm.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);

TheBlueMatt commented at 3:16 PM on October 28, 2016:

Can you move this up to cover the MESSAGE_SIZE_OFFSET write two lines up?

theuni commented at 5:40 PM on October 28, 2016:

ok

in src/net.cpp:None in f14fc2e48a outdated

2588 |  
2589 | -    LogPrint("net", "(aborted)\n");
2590 |  }
2591 |  
2592 | -void CNode::EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend)
2593 | +void CConnman::PushMessage(CNode* pnode, CDataStream& strm, const std::string& sCommand)

TheBlueMatt commented at 3:20 PM on October 28, 2016:

strm can be const, I believe? Would be nice to go ahead and do that since I'd like to use this to create a cmpctblock fully-encoded message once and then just call this on each node that wants it.

theuni commented at 4:21 PM on October 28, 2016:

ok

theuni commented at 7:55 PM on October 28, 2016:

Er, this isn't intended to be used that way. This is internal and should be private.

The next set of changes will introduce a CSendMessage which is fully serialized. PushMessage will take that instead. That fixes the current layer violation here, and your issue at the same time I think?

TheBlueMatt commented at 9:17 PM on October 28, 2016:

OK, sounds good.

in src/net.cpp:None in 22d4b518ae outdated

 413 | @@ -411,6 +414,24 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 414 |      return NULL;
 415 |  }
 416 |  
 417 | +void CConnman::PushVersion(CNode* pnode, int64_t nTime)

TheBlueMatt commented at 3:46 PM on October 28, 2016:

nit: I'd kinda prefer we not move PushVersion twice.

in src/main.cpp:None in f14fc2e48a outdated

 369 | +    CAddress addr = pnode->addr;
 370 | +
 371 | +    CAddress addrYou = (addr.IsRoutable() && !IsProxy(addr) ? addr : CAddress(CService(), addr.nServices));
 372 | +    CAddress addrMe = CAddress(CService(), nLocalNodeServices);
 373 | +
 374 | +    connman.PushMessageWithVersion(pnode, INIT_PROTO_VERSION, NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalNodeServices, GetTime(), addrYou, addrMe,

TheBlueMatt commented at 4:10 PM on October 28, 2016:

This is a slight change in behavior - we are now pushing our non-adjusted time to all peers, instead of just outbound peers. Not actually sure if it matters, but taking note here.

theuni commented at 4:23 PM on October 28, 2016:

Whoops, good catch. That was not intentional. Will fix.

in src/main.cpp:None in f14fc2e48a outdated

 361 | -    state.name = pnode->addrName;
 362 | -    state.address = pnode->addr;
 363 | +void PushNodeVersion(CNode *pnode, CConnman& connman)
 364 | +{
 365 | +    ServiceFlags nLocalNodeServices = pnode->GetLocalServices();
 366 | +    uint64_t nonce = pnode->GetLocalNonce();

TheBlueMatt commented at 4:15 PM on October 28, 2016:

I think half the functions here need locks or to become atomics to be correct. (unless I'm missing a required lock to call this function, in which case please AssertLockHeld to make it obvious).

May be easiest to just make half the ints in CNode atomic.

theuni commented at 4:23 PM on October 28, 2016:

This value doesn't change. I have a branch with additional commits that turns a bunch of stuff into const to make it clear that it's threadsafe. I could add those here if you'd like?

TheBlueMatt commented at 9:20 PM on October 28, 2016:

I'm ok with this happening in a separate PR, as AFAICT there are already dragons here, but should definitely happen before 0.14.

TheBlueMatt commented at 4:15 PM on October 28, 2016: member

A few nits, and some things that look like missing locks to me - I think we should do a pass over CNode and just convert a bunch of the various ints to std::atomic to fix most of the issues (ie they aren't issues on at least x86 right now).

in src/net.h:None in f14fc2e48a outdated

 758 | +        // only one version message is allowed per session. We can therefore
 759 | +        // treat this value as const and even atomic as long as it's only used
 760 | +        // once the handshake is complete. Any attempt to set this twice is an
 761 | +        // error.
 762 | +        assert(nSendVersion == 0);
 763 | +        *const_cast<int*>(&nSendVersion) = nVersionIn;

TheBlueMatt commented at 4:33 PM on October 28, 2016:

This violates C++ spec - nSendVersion must exist non-const somewhere for const_cast to be permitted (I believe).

theuni commented at 5:31 PM on October 28, 2016:

Ok, I'll just change this back to non-const. The const was only to prove that it's not racy, since it can only be set once.

theuni force-pushed on Oct 29, 2016

theuni commented at 8:38 PM on October 29, 2016: member

Updated for @TheBlueMatt's nits. Since the changes were very small, I went ahead and squashed.

PRs are coming up to address the const issues, as well as fixing the serialization layer violation.

in src/net.cpp:None in 2df814b2ec outdated

2579 | -
2580 | -    LEAVE_CRITICAL_SECTION(cs_vSend);
2581 | +    // Set the size
2582 | +    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2583 | +    WriteLE32((uint8_t*)&strm[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize);
2584 | +    assert(strm.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);

TheBlueMatt commented at 11:22 PM on October 31, 2016:

This assert needs to move up a line.

in src/net.cpp:None in 2df814b2ec outdated

2620 | -    // Set the checksum
2621 | -    uint256 hash = Hash(ssSend.begin() + CMessageHeader::HEADER_SIZE, ssSend.end());
2622 | -    assert(ssSend.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);
2623 | -    memcpy((char*)&ssSend[CMessageHeader::CHECKSUM_OFFSET], hash.begin(), CMessageHeader::CHECKSUM_SIZE);
2624 | +    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2625 | +    LogPrint("net", "%s (%d bytes) peer=%d\n", sCommand.c_str(), nSize, pnode->id);

TheBlueMatt commented at 11:23 PM on October 31, 2016:

nit: any reason you changed the text here? It used to read "sending: %s (%d bytes) peer=%d\n", SanitizeString(pszCommand, nSize, id) (split across two lines).

theuni commented at 9:08 PM on November 1, 2016:

No, they just ended up combined since the actual data push is all done in one place now.

Is there a reason to prefer having them split? How about adding back the "sending" for easy grepping, and the sanitizestring for extra paranoia, but leaving it as a combined message?

in src/net.cpp:None in 2df814b2ec outdated

1136 | @@ -1156,10 +1137,6 @@ void CConnman::ThreadSocketHandler()
1137 |                  {
1138 |                      TRY_LOCK(pnode->cs_vSend, lockSend);
1139 |                      if (lockSend) {
1140 | -                        if (pnode->nOptimisticBytesWritten) {

TheBlueMatt commented at 12:07 AM on November 1, 2016:

I cant see anything wrong with this, but it seems obviously equivalent...what was the reason for nOptimisticBytesSent previously?

theuni commented at 9:15 PM on November 1, 2016:

It was a hack. In fact, this PR was originally created to fix this.

If the node is in charge of pushing data, Connman has no way of knowing if optimistic writes have succeeded. The hackish solution was to cache the periodically check for optimistic written bytes (stored by CNode) and add them to the node's total, but that fails miserably if the optimistic writes succeed most of the time.

Now that CConnman is in charge of the writes, it knows immediately how much was written, and doesn't have to try to catch up with it later.

TheBlueMatt commented at 2:56 PM on November 1, 2016: member

Aside from the few comments, utACK 2df814b2ecfcfe67f5a7419b662df1ec1efd9d2d.

TheBlueMatt commented at 9:44 PM on November 1, 2016: member

Oh totally, didn't mean to imply the sending message should be two prints, just that you changed the text spuriously.

On November 1, 2016 5:08:13 PM EDT, Cory Fields notifications@github.com wrote:

theuni commented on this pull request.

// Set the checksum

uint256 hash = Hash(ssSend.begin() + CMessageHeader::HEADER_SIZE, ssSend.end());

assert(ssSend.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);

memcpy((char*)&ssSend[CMessageHeader::CHECKSUM_OFFSET], hash.begin(), CMessageHeader::CHECKSUM_SIZE);

unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;

LogPrint("net", "%s (%d bytes) peer=%d\n", sCommand.c_str(), nSize, pnode->id);

No, they just ended up combined since the actual data push is all done in one place now.

Is there a reason to prefer having them split? How about adding back the "sending" for easy grepping, and the sanitizestring for extra paranoia, but leaving it as a combined message?

You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #8708

in src/net.h:None in 19d3daef93 outdated

 839 | -    void EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend);
 840 | -
 841 | -    void PushVersion();
 842 | -
 843 | -
 844 | -    void PushMessage(const char* pszCommand)

laanwj commented at 8:42 PM on November 2, 2016:

Good to get rid of this repetition.

in src/net.cpp:None in 19d3daef93 outdated

2593 | +void CConnman::PushMessage(CNode* pnode, CDataStream& strm, const std::string& sCommand)
2594 |  {
2595 | -    // The -*messagestest options are intentionally not documented in the help message,
2596 | -    // since they are only used during development to debug the networking code and are
2597 | -    // not intended for end-users.
2598 | -    if (mapArgs.count("-dropmessagestest") && GetRand(GetArg("-dropmessagestest", 2)) == 0)

laanwj commented at 8:45 PM on November 2, 2016:

Yeah, I don't think anyone is using these, or has been using these for years. Would be nice to get #7940 in at some point, it provides a better fuzzing framework which is not dependent on the network code.

theuni commented at 9:52 PM on November 2, 2016:

@laanwj looks like I missed #7940. Agree with doing it at a lower level.

in src/net.h:None in 19d3daef93 outdated

 135 | @@ -136,6 +136,33 @@ class CConnman
 136 |  
 137 |      bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
 138 |  
 139 | +    template <typename... Args>

laanwj commented at 8:51 PM on November 2, 2016:

These are not used?

theuni commented at 10:03 PM on November 2, 2016:

PushMessageWithVersionAndFlag could be private I suppose. It's only used by the others.

PushMessageWithFlag is the same as before.

PushMessageWithVersion is used explicitly before the handshake is complete, before we know what upgraded version to use.

theuni commented at 10:10 PM on November 2, 2016:

Note that as next steps, an intermediary and fully serialized "CSendMessage" (or so) class will be added, which CConnman accepts rather than bare arguments, and forwards to the network. This makes CConnman mostly oblivious as to what it's pushing (other than the type of message), which further improves encapsulation.

Also, The sendversion logic will move to CNodeState once the locking situation there is improved.

laanwj commented at 8:51 PM on November 2, 2016: member

How does this overlap with #9039? That pull changes the serialization framework to get rid of nVersion and nType in many places, does that mean some things can be simpllified here?

sipa commented at 9:56 PM on November 2, 2016: member

I believe the removal of CNode::Fuzz maybe remove the need for CDataStream::insert or CDataStream::erase.

sipa commented at 9:57 PM on November 2, 2016: member

As for overlap with #9039, I expect it to be trivial: just dropping the nVersion and nType everywhere. I'm happy to rebase #9039 on top of this if it gets merged first.

theuni commented at 11:07 PM on November 2, 2016: member

Agree with @sipa. Either order is fine by me.

in src/net.cpp:None in 01fec5473e outdated

2686 | @@ -2686,6 +2687,52 @@ void CNode::EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend)
2687 |      LEAVE_CRITICAL_SECTION(cs_vSend);
2688 |  }
2689 |  
2690 | +CDataStream CConnman::BeginMessage(CNode* pnode, int nVersion, int flags, const std::string& sCommand)
2691 | +{
2692 | +    return {SER_NETWORK, nVersion ? nVersion | flags : pnode->GetSendVersion() | flags, CMessageHeader(Params().MessageStart(), sCommand.c_str(), 0) };

sipa commented at 6:41 AM on November 3, 2016:

That's pretty hard to read. Can you factor the version logic out, or at least combine as (nVersion ? nVersion : pnode->GetSendVersion()) | flags?

theuni commented at 1:57 PM on November 3, 2016:

Will do.

in src/net.cpp:None in 01fec5473e outdated

2717 | +        LOCK(pnode->cs_vSend);
2718 | +        if(pnode->hSocket == INVALID_SOCKET) {
2719 | +            return;
2720 | +        }
2721 | +        bool optimisticSend(pnode->vSendMsg.empty());
2722 | +        pnode->vSendMsg.emplace_back(strm.begin(), strm.end());

sipa commented at 6:47 AM on November 3, 2016:

Not for this PR, but we should try to make a CSerializeData move-constructable from a CDataStream, and make PushMessage take a CDataStream&& perhaps. Or is this going away anyway?

theuni commented at 2:02 PM on November 3, 2016:

Yea, going away. Instead we'll have a new (very simple) stream type that serializes args for the network, which CConnman will use directly. As it'll construct in-place and be movable, it should eliminate at least 2 copies, I think.

in src/main.cpp:None in 6366b1054e outdated

 354 | @@ -355,10 +355,10 @@ void UpdatePreferredDownload(CNode* node, CNodeState* state)
 355 |  }
 356 |  
 357 |  void InitializeNode(NodeId nodeid, const CNode *pnode) {
 358 | +    CAddress addr = pnode->addr;

sipa commented at 6:54 AM on November 3, 2016:

Why copy pnode->addr twice?

sipa commented at 6:59 AM on November 3, 2016: member

utACK

serialization: teach serializers variadics

Also add a variadic CDataStream ctor for ease-of-use.

db78f311ba

connman is in charge of pushing messages

The changes here are dense and subtle, but hopefully all is more explicit
than before.

- CConnman is now in charge of sending data rather than the nodes themselves.
  This is necessary because many decisions need to be made with all nodes in
  mind, and a model that requires the nodes calling up to their manager quickly
  turns to spaghetti.

- The per-node-serializer (ssSend) has been replaced with a (quasi-)const
  send-version. Since the send version for serialization can only change once
  per connection, we now explicitly tag messages with INIT_PROTO_VERSION if
  they are sent before the handshake. With this done, there's no need to lock
  for access to nSendVersion.

  Also, a new stream is used for each message, so there's no need to lock
  during the serialization process.

- This takes care of accounting for optimistic sends, so the
  nOptimisticBytesWritten hack can be removed.

- -dropmessagestest and -fuzzmessagestest have not been preserved, as I suspect
  they haven't been used in years.

462965635e

net: switch all callers to connman for pushing messages

Drop all of the old stuff.

a6eff7cf73

drop the optimistic write counter hack

This is now handled properly in realtime.

7f10bc8a76

net: remove now-unused ssSend and Fuzz 22f2af6997

net: construct CNodeStates in place 6ba267c170

net: handle version push in InitializeNode c7b8effb96

theuni force-pushed on Nov 3, 2016

theuni commented at 2:15 PM on November 3, 2016: member

Rebased after #9050. The net.cpp diff before/after the rebase:

git diff 19d3daef936905743ef108aee1ccfe46fd77b283..c7b8effb96726ba5367a7b1c9c7374c4d2cc10e2 -- net.cpp

can be seen here: https://gist.github.com/theuni/cc4eb87f0196ddadb637d1a41c5c5b2e.

It's just #9050 plus @sipa's simplification suggestion.

TheBlueMatt commented at 8:17 PM on November 3, 2016: member

Looks like it needs rebased again, sorry :/.

sipa commented at 9:05 PM on November 3, 2016: member

The rebase is trivial: https://github.com/sipa/bitcoin/commits/pr_8708

laanwj referenced this in commit c8c572f8f1 on Nov 7, 2016

laanwj commented at 9:37 AM on November 7, 2016: member

Was merged via c8c572f8f1ea0a98ee3e7b3343c766ad52848bef (from sipa's rebased branch https://github.com/sipa/bitcoin/commits/pr_8708)

laanwj closed this on Nov 7, 2016

theuni commented at 6:58 PM on November 7, 2016: member

Thanks!

in src/main.cpp:None in c7b8effb96

  17 | @@ -18,6 +18,7 @@
  18 |  #include "init.h"
  19 |  #include "merkleblock.h"
  20 |  #include "net.h"
  21 | +#include "netbase.h"

rebroad commented at 12:52 PM on November 11, 2016:

what does it use in netbase.h?

MarcoFalke commented at 2:04 PM on November 11, 2016:

@rebroad An easy way to find out is to remove the line and compile. The compiler will tell you what is used but missing.

zkbot referenced this in commit 3b68ab255f on Apr 17, 2018

zkbot referenced this in commit d408e23ab7 on Apr 18, 2018

zkbot referenced this in commit 0753a0e8a9 on Apr 19, 2018

meshcollider moved this from the "In progress" to the "Done" column in a project

random-zebra referenced this in commit cbd9271afb on Sep 7, 2020

MarcoFalke locked this on Sep 8, 2021