Skip to content
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

Overwinter peer management #2919

Merged
merged 3 commits into from Feb 24, 2018
Merged

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented Jan 29, 2018

Implements ZIP 201.

Part of #2904.

@bitcartel bitcartel added this to the 1.0.15 milestone Jan 29, 2018
@bitcartel bitcartel self-assigned this Jan 29, 2018
@bitcartel bitcartel requested a review from str4d Jan 29, 2018
Copy link
Contributor

str4d left a comment

Initial musings. I'll go over the ZIP again later and do another round of thinking.

vRecv >> pfrom->nBranchId;

// FIXME: Is this branch ID acceptable to this node? Set of acceptable branchIDs?
NUInfo info = NetworkUpgradeInfo[Consensus::UPGRADE_TESTDUMMY];

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

We have three options:

  1. Branch ID corresponding to the peer's current chain tip
  2. Branch ID corresponding to the peer's desired chain tip (ie. the last branch they will follow)
  3. Set of branch IDs that the peer will follow.

3 is the most flexible, in that it enables us to reason about whether the peer follows most of our chain but not all of it. Given that we decided we weren't adding that complexity to Overwinter, however, I think 2 is what we want. We can rely on the assumption that branch IDs are globally random, so if the peer doesn't provide the same branch ID as you expect to reach, even if it is one you know about (having been on previously), you can be sure that they will never reach your branch. Conversely, if a peer advertises a branch ID for a new branch that you don't know about, you can be sure that you will never reach that branch (and in fact without 3, the two cases are indistinguishable).

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Oh, I completely forgot that the original intent of this field was to disambiguate the protocol version, which adds another option:

  1. Branch ID corresponding to the branch this protocol version was introduced for (Overwinter) or during (Sprout).

However, thinking about this more, we never actually parse data conditional on the protocol version in the handshake (since we could be talking to a supportable older node). So we might need two (or three) things:

  • Protocol format branch ID
  • Peer's consensus branch ID (one of the three options above)
  • Possibly something else to enable reasonable versioning/parsing of the handshake message itself (but this might be satisfied by the protocol format branch ID).

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 3, 2018

Author Contributor

@str4d the protocol version branch id - is this the branch id when the protocol version first goes live?

static_assert(PROTOCOL_VERSION >= OVERWINTER_PROTO_VERSION, "protocol version must be >= Overwinter protocol version");

if (!vRecv.empty()) {
vRecv >> pfrom->nBranchId;

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Given the comment below, and the fact that we've now defined branch ID zero to mean "Sprout", we should just always attempt to read this, while leaving the forced disconnection inside the NetworkUpgradeActive() check.

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

See comments below about protocol handshake versioning.

src/net.cpp Outdated
const Consensus::Params& params = Params().GetConsensus();
int nActivationHeight = params.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight;

if ((height <= nActivationHeight) &&

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

height < nActivationHeight, since the new rules are live from nActivationHeight.

src/net.cpp Outdated

// Priotize these nodes by replacing eviction set with them
if (vTmpEvictionCandidates.size() > 0) {
vEvictionCandidates = vTmpEvictionCandidates;

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

This could be used by an attacker to trigger an early network split (up to) two days before the actual branch height. We should instead ensure that both Overwinter and Sprout nodes are protected, but at some ratio (3:1? 4:1?), so we get the churn movement we want without risking an early split.

A simple way to achieve that would be to implement a comparer (a la CompareNetGroupKeyed etc.) that sorts by protocol version (or branch ID, depending on decided semantics), and then erase the first 4 and last 1 (or erase last 4, reverse, erase last 1, if that is more efficient).

After writing the above, I realised that the netgroup protection below effectively provides this (protecting some of the remaining Sprout nodes), so it's probably fine to leave this as-is. Leaving my comment intact for review context.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 2, 2018

Author Contributor

Looking at the netgroup code, Sprout nodes should eventually whittle down to just one connection.

    // Do not disconnect peers if there is only one unprotected connection from their network group.
    if (vEvictionCandidates.size() <= 1)
pfrom->nVersion != 0 &&
NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_TESTDUMMY))
{
if (pfrom->nVersion < OVERWINTER_PROTO_VERSION) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

This should be fine, as we can be sure that any version below this is undesired, regardless of what branch it is from.

pfrom->fDisconnect = true;
return false;
}
else if (pfrom->nBranchId != NetworkUpgradeInfo[Consensus::UPGRADE_TESTDUMMY].nBranchId) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

If nBranchId ends up being the branch ID of the peer's chain tip at time of connect, we need to think about how it gets updated (new network messages?)

If it ends up being the branch ID the peer will eventually target, I think this should be fine, because to change that, the peer will need to drop its connection and restart.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 2, 2018

Author Contributor

A node only ever sends the version message once...

So the peers could end up targeting different branch ids. In that situation, at some point, the network messages or most likely the transactions being relayed will be incompatible, and the peer will be dropped.

We could add the requirement that when Overwinter is active, pong messages should include the node's current branch id. If the receiving node doesn't support that branch id then disconnect, however, that might mean a node which is catching up and going through different branch ids gets dropped before it can sync up.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 4, 2018

Note on updating mininode.py to handle Overwinter. The following tests use TestManager which serialize and deserialize the version message.

  • bip65-cltv-p2p.py
  • bipdersig-p2p.py
  • invalidblockrequest.py
  • maxblocksinflight.py
  • script_test.py (disabled test)
@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from b720fb1 to f691da1 Feb 4, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 4, 2018

Pushed update to rebase on activation logic, modify mininode and address review comments.
Branch id discussion in comments needs to be resolved.
[Update] Force pushed after discussion where we decided not to send the branch id as part of the version message, which simplifies the code.

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch 2 times, most recently from 58c3a76 to f075903 Feb 6, 2018
@bitcartel bitcartel changed the title [WIP] 2904 branch id protocol handshake [WIP] 2904 Overwinter version handshake Feb 8, 2018
@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from f075903 to af159ef Feb 8, 2018
@str4d str4d added the NU0 wishlist label Feb 12, 2018
Copy link
Contributor

str4d left a comment

This PR currently evicts peers preferentially ahead of Overwinter, and disconnects existing old peers after Overwinter, but it doesn't reject incoming connections from old peers after Overwinter. These peers will be caught later by the existing peers check, so we may as well drop them early.

Add a pfrom->nVersion < OVERWINTER_PROTO_VERSION && NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER) check just after the MIN_PEER_PROTO_VERSION check in the version message handling.

@@ -39,6 +39,7 @@
zcash_person,
)

OVERWINTER_PROTO_VERSION = 170003

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

This isn't used anywhere. Is there a missing test, or is this preparatory?

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 17, 2018

Author Contributor

Preparatory

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 20, 2018

Author Contributor

And missing test.

// 4. Overwinter is active
else if (
MIN_PEER_PROTO_VERSION < OVERWINTER_PROTO_VERSION &&
pfrom->nVersion != 0 &&

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

This condition fragment cannot be false, as there is a pfrom->nVersion == 0 check above here.


// Prioritize these nodes by replacing eviction set with them
if (vTmpEvictionCandidates.size() > 0) {
vEvictionCandidates = vTmpEvictionCandidates;

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

As the number of connected pre-Overwinter peers decreases, this could result in a condition where all pre-Overwinter peers are protected by the subsequent checks, so we stop evicting anyone earlier (with more connected peers) than we otherwise would. I don't believe this can be abused (as the Overwinter peers being protected will almost certainly include peers that were protected before this period began, so the adversary should gain no advantage), but we should keep an eye on this behaviour while testing.

@str4d str4d changed the title [WIP] 2904 Overwinter version handshake Overwinter peer management Feb 16, 2018
@str4d str4d added this to Work Queue in Network Upgrade 0 via automation Feb 16, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 20, 2018

@str4d @daira There is a magic value in this PR. Okay or change? Currently set to ~ 3 days.

/** The period before a network upgrade activates, where connections to upgrading peers is preferred (in blocks). */
static const int NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PERIOD = 24 * 24 * 3;
@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from af159ef to b7e6896 Feb 20, 2018
Copy link
Contributor

str4d left a comment

Needs a test, which should be implementable using MiniNode.

src/net.cpp Outdated
// Find any nodes which don't support Overwinter protocol version
BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) {

// FIXME: version field of node should be set by now, but we could check for 0 too.

This comment has been minimized.

Copy link
@str4d

str4d Feb 20, 2018

Contributor

Nit: indentation.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 20, 2018

Author Contributor

Fixed

src/net.cpp Outdated
}

const Consensus::Params& params = Params().GetConsensus();
int nActivationHeight = params.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight;

This comment has been minimized.

Copy link
@str4d

str4d Feb 20, 2018

Contributor

Consensus::UPGRADE_OVERWINTER

src/net.h Outdated
@@ -57,6 +57,8 @@ static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
/** The maximum number of peer connections to maintain. */
static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
/** The period before a network upgrade activates, where connections to upgrading peers is preferred (in blocks). */
static const int NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PERIOD = 24 * 24 * 3;

This comment has been minimized.

Copy link
@str4d

str4d Feb 20, 2018

Contributor

3 days seems fine for 1.0.15; it should give us enough time to examine the effects on testnet. We should revisit the value before 1.1.0.

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from b7e6896 to db24320 Feb 20, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 20, 2018

Forced pushed to address review comments. Now working on mininode test.

@@ -15,6 +15,7 @@
#include "checkqueue.h"
#include "consensus/upgrades.h"
#include "consensus/validation.h"
#include "consensus/upgrades.h"

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

Remove (already included a few lines earlier).

@@ -11,6 +11,9 @@

static const int PROTOCOL_VERSION = 170002;

//! Overwinter proto version
static const int OVERWINTER_PROTO_VERSION = 170003;

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

This will need to be bumped when we set the mainnet activation height.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

To 170004? - because we set 170003 for 1.0.15 which doesn't have a mainnet activation height?

This comment has been minimized.

Copy link
@ebfull

ebfull Feb 21, 2018

Contributor

There needs to be a ticket or some process written down specifying this.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

@ebfull there is a ticket for the testnet bump, but you're right, we should have this documented somewhere. #2969

NetworkThread().start()

# Sprout consensus rules apply after mining 9 blocks
self.nodes[0].generate(9)

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

I think there's an off-by-one error. Per ZIP 200, ACTIVATION_HEIGHT - 1 is the last common block, so once we receive that block, we don't want to receive the next block that non-upgrading nodes broadcast.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

I don't think there's an off-by-one. Just the test comments were not clear. The zcash node in self.nodes[0] has an activation height of 10. At block height 9, the test confirms that Sprout and Overwinter mininodes (which advertise version 170002 and 170003) are all connected to the zcash node. The mininode connections don't care about blocks; only whether or not the mininode can connect and/or remain connected.

@@ -4623,6 +4624,17 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
return false;
}

// When Overwinter is active, reject incoming connections from non-Overwinter nodes
if (NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER)

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

GetHeight() + 1 to address the off-by-one error.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

I don't think there's an off-by-one error. At block height 9, Sprout rules still apply, thus a Sprout node should be allowed to connect; perhaps the node is syncing and makes a request for block data.

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

It's a tricky one: Sprout rules apply to block height 9 itself, but Overwinter rules apply to blocks after height 9. So it depends on whether you consider Overwinter to be "active" once the last Sprout block has been received, or only once the first Overwinter block has been received. I think the latter is clearly incorrect (because the mempool needs to enforce Overwinter rules at height 9 in preparation for block 10), but I'm not sure the former is obviously or intuitively correct.

I think it makes sense to split the concept of "active-ness" into two categories:

  • Consensus rules (obviously Overwinter rules apply only when block 10 is received)
  • Non-consensus behaviours (at height 9 mempool only accepts Overwinter transactions, wallet will create Overwinter transactions, etc.)

I think the network behaviour falls into the latter category.

This affects #2808 because it needs to provide the status of each upgrade in a way that users of getblockchaininfo can understand and act on. If I were to think about a third party making transactions, or deciding at what point to start doing Overwinter things, I think I'd want to have an indication of the latter? Or more specifically, I'd want to have "Overwinter is active" occur right at the point when the block at height 9 has been fully-validated and accepted. This doesn't mesh with the way statuses are currently defined by NetworkUpgradeStatus, which assumes that "Overwinter is active" occurs right at the point when the block at height 10 has been fully-validated and accepted (which is when chainActive.Height() will start returning 10).

This comment has been minimized.

Copy link
@ebfull

ebfull Feb 21, 2018

Contributor

In general we should consider Overwinter "active" once the last possible Sprout height has been reached.

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

ZIP 200 states:

ACTIVATION_HEIGHT

The non-zero block height at which the network upgrade rules will come
into effect, and be enforced as part of the blockchain consensus.

For removal of ambiguity, the block at height ACTIVATION_HEIGHT - 1 is
subject to the pre-upgrade consensus rules, and would be the last common
block in the event of a persistent pre-upgrade branch.

If we consider a network upgrade to be "active" once the last possible height for the previous branch is reached, I think this would become:

ACTIVATION_HEIGHT

The non-zero block height at which the network upgrade rules will come
into effect, and be enforced as part of the blockchain consensus.

For removal of ambiguity, the block at height ACTIVATION_HEIGHT is itself
subject to the pre-upgrade consensus rules, and would be the last common
block in the event of a persistent pre-upgrade branch. The block at height
ACTIVATION_HEIGHT + 1 will be the first block to have the network upgrade
rules enforced.

Is this a more sensible and/or understandable definition? Which one is prone to less confusion?

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Feb 21, 2018

Contributor

Yeah, seems NetworkUpgradeState(upgrade) https://github.com/zcash/zcash/pull/2898/files#diff-a95ee8b8cd71c9d70d87b4d0e70f833eR29 should return a value according to whether the next block is in the range [L,R) where L=activation height of upgrade, R =activation height of next upgrade.

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Feb 21, 2018

Contributor

Ohh syncing problems my "yeah" was to your previous comment and Sean's. Regarding your last comment, I don't think the definition of ACTIVATION_HEIGHT should change to the previous block, but the definition of when the upgrade is active should be according to what I wrote.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

In general we should consider Overwinter "active" once the last possible Sprout height has been reached.

Can it be "active" if the activation height block never arrives? Someone looking at the chain would see 100% Sprout blocks.

I think the ZIP is fine as is; perhaps add a paragraph distinguishing between the consensus rule activation event when the new block arrives, and when a node should alter its behaviour to prepare for that event.

We know that at ACTIVATION_HEIGHT - 1 a node (can|should) change its behaviour to ensure a user action is executed as intended. Creating a transaction will be version 3 so it can get into the next block. Rejecting version 1 and 2 transactions from the mempool helps miners using getblocktemplate.

So should Overwinter nodes drop Sprout nodes at the ACTIVATION_HEIGHT or in the block before it? Currently at the activation height we (1) reject incoming sprout connections (2) disconnect existing sprout connections when a message is sent, and before the activation height, we have some code which tries to maintain connections with Overwinter nodes rather than Sprout nodes in the run-up to activation, by prioritizing Sprout nodes for eviction.

This comment has been minimized.

Copy link
@daira

daira Feb 22, 2018

Contributor

I think either behaviour is reasonable. There's no harm in dropping Sprout nodes once the block before ACTIVATION_HEIGHT is connected (but obviously the test would need to be changed in that case).

// 2. Overwinter is active
// 3. Peer version is pre-Overwinter
// 4. Peer branch id is not supported
else if (NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER))

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

GetHeight() + 1 to address the off-by-one error.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

Ditto above.

This comment has been minimized.

Copy link
@str4d

str4d Feb 22, 2018

Contributor

Here's the other bug: this conditional eats all messages after Overwinter activates. It needs to be combined with the inner conditional, same as the check during version parsing.

src/net.cpp Outdated
const Consensus::Params& params = Params().GetConsensus();
int nActivationHeight = params.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight;

if ((height < nActivationHeight) &&

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

This might need to be height + 1 < nActivationHeight to match the above changes.

This comment has been minimized.

Copy link
@str4d

str4d Feb 21, 2018

Contributor

(or height = chainActive.Height() + 1 further up)

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 21, 2018

Author Contributor

Ditto above.

This comment has been minimized.

Copy link
@str4d

str4d Feb 22, 2018

Contributor

I was momentarily concerned that this might have a wrapping problem, but both height and nActivationHeight are ints, and nActivationHeight == -1 for mainnet in 1.0.15, so height < nActivationHeight will always evaluate to false.

This comment has been minimized.

Copy link
@daira

daira Feb 22, 2018

Contributor

It would nevertheless be clearer to write if ((nActivationHeight >= 0) && ....

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from dc75062 to 68d9180 Feb 21, 2018
@bitcartel bitcartel requested a review from daira Feb 21, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 22, 2018

At the moment, the PR is in sync with the ZIP zcash/zips#134 re: dropping existing and rejecting new Sprout connections once the first Overwinter block has been received i.e. activated.

@str4d
str4d approved these changes Feb 22, 2018
Copy link
Contributor

str4d left a comment

utACK. I'm withdrawing my earlier off-by-one objection, and will keep an eye on the testnet activation behaviour instead.

@ebfull ebfull self-requested a review Feb 22, 2018
@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from 5204a61 to 80c4b2b Feb 23, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 23, 2018

Pushed update for review comments from @daira. Here's the diff:

diff --git a/src/net.cpp b/src/net.cpp
index c1e87b8..37fd6ef 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -824,8 +824,9 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
     const Consensus::Params& params = Params().GetConsensus();
     int nActivationHeight = params.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight;
 
-    if ((height < nActivationHeight) &&
-        (height >= (nActivationHeight - NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PERIOD)))
+    if (nActivationHeight > 0 &&
+        height < nActivationHeight &&
+        height >= nActivationHeight - NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PERIOD)
     {
         // Find any nodes which don't support Overwinter protocol version
         BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) {
diff --git a/src/net.h b/src/net.h
index 54c1340..55190ba 100644
--- a/src/net.h
+++ b/src/net.h
@@ -57,7 +57,7 @@ static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
 static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
 /** The maximum number of peer connections to maintain. */
 static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
-/** The period before a network upgrade activates, where connections to upgrading peers is preferred (in blocks). */
+/** The period before a network upgrade activates, where connections to upgrading peers are preferred (in blocks). */
 static const int NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PERIOD = 24 * 24 * 3;
 
 unsigned int ReceiveFloodSize();
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Feb 23, 2018

Here's a proposal for fixing the versioning inconsistency without touching parts of the codebase we haven't been reviewing in these Overwinter PRs:

  • Move nProtocolVersion to Consensus::Params::vUpgrades (alongside nActivationHeight)
  • Set it to 170004 for mainnet, and 170003 for testnet.
  • Use chainparams.GetConsensus().vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion as the version to compare the peer's protocol version to.
  • Set PROTOCOL_VERSION to 170003.

This means that mainnet peers will be, as of 1.0.15, checking against version 170004, but this is fine because those checks are gated by Overwinter activation checks anyway. Meanwhile testnet will behave as currently implemented for 1.0.15, and then in 1.1.0 we set PROTOCOL_VERSION = 170004 and they still behave fine because for testnet, because ...vUpgrades[...OVERWINTER].nProtocolVersion will still resolve to 170003.

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from 80c4b2b to 9a43845 Feb 23, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 23, 2018

Pushed update based on discussion above. I have archived previous version here: https://github.com/bitcartel/zcash/tree/2904_branch_id_protocol_handshake_old_nuinfo_struct

@daira Does this address your comment here? #2969 (comment)

Here is the diff from last review round.

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index a0c73dc..63f1ab7 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -101,6 +101,9 @@ public:
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170004;
 
         /**
          * The message start string should be awesome! ⓩ❤
@@ -259,6 +262,9 @@ public:
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
 
         pchMessageStart[0] = 0xfa;
         pchMessageStart[1] = 0x1a;
@@ -366,6 +372,9 @@ public:
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
 
         pchMessageStart[0] = 0xaa;
         pchMessageStart[1] = 0xe8;
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 53f8609..1fcf35f 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -29,6 +29,11 @@ enum UpgradeIndex {
 
 struct NetworkUpgrade {
     /**
+     * The first protocol version for which the new consensus rules will be active
+     */
+    int nProtocolVersion;
+
+    /**
      * Height of the first block for which the new consensus rules will be active
      */
     int nActivationHeight;
diff --git a/src/consensus/upgrades.cpp b/src/consensus/upgrades.cpp
index a67ce6f..17606bc 100644
--- a/src/consensus/upgrades.cpp
+++ b/src/consensus/upgrades.cpp
@@ -13,19 +13,16 @@ const struct NUInfo NetworkUpgradeInfo[Consensus::MAX_NETWORK_UPGRADES] = {
         /*.nBranchId =*/ 0,
         /*.strName =*/ "Sprout",
         /*.strInfo =*/ "The Zcash network at launch",
-        /*.nProtocolVersion =*/ 170002,
     },
     {
         /*.nBranchId =*/ 0x74736554,
         /*.strName =*/ "Test dummy",
         /*.strInfo =*/ "Test dummy info",
-        /*.nProtocolVersion =*/ 170002,
     },
     {
         /*.nBranchId =*/ 0x5ba81b19,
         /*.strName =*/ "Overwinter",
         /*.strInfo =*/ "TBD",
-        /*.nProtocolVersion =*/ 170003,
     }
 };
 
diff --git a/src/consensus/upgrades.h b/src/consensus/upgrades.h
index d8caf78..6a91732 100644
--- a/src/consensus/upgrades.h
+++ b/src/consensus/upgrades.h
@@ -22,8 +22,6 @@ struct NUInfo {
     std::string strName;
     /** User-facing information string about the upgrade */
     std::string strInfo;
-    /** First protocol version which understands the upgrade */
-    int nProtocolVersion;
 };
 
 extern const struct NUInfo NetworkUpgradeInfo[];
diff --git a/src/main.cpp b/src/main.cpp
index 99629ba..621e05d 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4722,13 +4722,14 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
         }
 
         // When Overwinter is active, reject incoming connections from non-Overwinter nodes
-        if (NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER)
-            && pfrom->nVersion < NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nProtocolVersion)
+        const Consensus::Params& params = Params().GetConsensus();
+        if (NetworkUpgradeActive(GetHeight(), params, Consensus::UPGRADE_OVERWINTER)
+            && pfrom->nVersion < params.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion)
         {
             LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, pfrom->nVersion);
             pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE,
                             strprintf("Version must be %d or greater",
-                            NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nProtocolVersion));
+                            params.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion));
             pfrom->fDisconnect = true;
             return false;
         }
@@ -4857,12 +4858,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
     // 3. Peer version is pre-Overwinter
     // 4. Peer branch id is not supported
     else if (NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER)
-            && (pfrom->nVersion < NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nProtocolVersion))
+            && (pfrom->nVersion < chainparams.GetConsensus().vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion))
     {
         LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, pfrom->nVersion);
         pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE,
                             strprintf("Version must be %d or greater",
-                            NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nProtocolVersion));
+                            chainparams.GetConsensus().vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion));
         pfrom->fDisconnect = true;
         return false;
     }
diff --git a/src/net.cpp b/src/net.cpp
index 37fd6ef..72ef82d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -830,7 +830,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
     {
         // Find any nodes which don't support Overwinter protocol version
         BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) {
-            if (node->nVersion < NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nProtocolVersion) {
+            if (node->nVersion < params.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion) {
                 vTmpEvictionCandidates.push_back(node);
             }
         }
@str4d
str4d approved these changes Feb 23, 2018
Copy link
Contributor

str4d left a comment

ut(ACK+cov)

@@ -101,6 +101,9 @@ class CMainParams : public CChainParams {
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170004;

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Non-blocking: I'd prefer to intersperse these, and match the ordering in params.h.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Author Contributor

ok doing it now

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Hmm, these are duplicated now.

@@ -28,6 +28,11 @@ enum UpgradeIndex {
};

struct NetworkUpgrade {
/**
* The first protocol version for which the new consensus rules will be active

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

The first protocol version which will understand the new consensus rules

// 1. The version message has been received
// 2. Overwinter is active
// 3. Peer version is pre-Overwinter
// 4. Peer branch id is not supported

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Remove (not implemented in this PR)

@str4d str4d dismissed ebfull’s stale review Feb 23, 2018

Previous iteration.

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from d89ceb7 to 6d566f7 Feb 23, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 23, 2018

Changes pushed, per latest review:

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 63f1ab7..f11d534 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -95,15 +95,15 @@ public:
         consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
         consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
         consensus.nPowTargetSpacing = 2.5 * 60;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
             Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170004;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
-        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170004;
 
         /**
          * The message start string should be awesome! ⓩ❤
@@ -256,15 +256,15 @@ public:
         consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
         consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
         consensus.nPowTargetSpacing = 2.5 * 60;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
             Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
-        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
 
         pchMessageStart[0] = 0xfa;
         pchMessageStart[1] = 0x1a;
@@ -366,15 +366,15 @@ public:
         consensus.nPowMaxAdjustDown = 0; // Turn off adjustment down
         consensus.nPowMaxAdjustUp = 0; // Turn off adjustment up
         consensus.nPowTargetSpacing = 2.5 * 60;
+        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
             Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
+        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
         consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
+        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
         consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
             Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
-        consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
-        consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;
 
         pchMessageStart[0] = 0xaa;
         pchMessageStart[1] = 0xe8;
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 1fcf35f..11504db 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -29,7 +29,7 @@ enum UpgradeIndex {
 
 struct NetworkUpgrade {
     /**
-     * The first protocol version for which the new consensus rules will be active
+     * The first protocol version which will understand the new consensus rules
      */
     int nProtocolVersion;
 
diff --git a/src/main.cpp b/src/main.cpp
index 621e05d..5d45a29 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4856,7 +4856,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
     // 1. The version message has been received
     // 2. Overwinter is active
     // 3. Peer version is pre-Overwinter
-    // 4. Peer branch id is not supported
     else if (NetworkUpgradeActive(GetHeight(), chainparams.GetConsensus(), Consensus::UPGRADE_OVERWINTER)
             && (pfrom->nVersion < chainparams.GetConsensus().vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion))
     {
@@ -101,6 +101,9 @@ class CMainParams : public CChainParams {
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170004;

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Hmm, these are duplicated now.

consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Ditto.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Author Contributor

strange will fix

consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::BASE_SPROUT].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nProtocolVersion = 170002;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nProtocolVersion = 170003;

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Ditto.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Author Contributor

strange will fix

@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from 6d566f7 to 8c20b97 Feb 23, 2018
@bitcartel bitcartel force-pushed the bitcartel:2904_branch_id_protocol_handshake branch from 8c20b97 to b6e5cc7 Feb 23, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Feb 23, 2018

Force push to clean up (1) duplication (2) and move adding to mininode.py the line OVERWINTER_PROTO_VERSION = 170003 from peer management commit to the qa test commit.

@str4d
str4d approved these changes Feb 23, 2018
Copy link
Contributor

str4d left a comment

ut(ACK+cov)

@daira
daira approved these changes Feb 23, 2018
Copy link
Contributor

daira left a comment

Excellent! ut(ACK+cov)

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Feb 24, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

📌 Commit b6e5cc7 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

⌛️ Testing commit b6e5cc7 with merge e6e483b...

zkbot added a commit that referenced this pull request Feb 24, 2018
…str4d

Overwinter peer management

Implements ZIP 201.

Part of #2904.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing e6e483b to master...

@zkbot zkbot merged commit b6e5cc7 into zcash:master Feb 24, 2018
1 check passed
1 check passed
homu Test successful
Details
Network Upgrade 0 automation moved this from Work Queue to Complete Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Network Upgrade 0
  
Complete
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.