Witness flag erroneously activated on PoS networks

[zeptin]

In the constructor of NetworkPeerConnectionParameters:

        public NetworkPeerConnectionParameters()
        {
            this.Version = ProtocolVersion.PROTOCOL_VERSION;
...
            this.PreferredTransactionOptions = TransactionOptions.All;

This seems to mean that, regardless of the protocol version defined in Program.cs for StratisD (70000), when handshaking with a peer we bump it up to PROTOCOL_VERSION (70012) and enable the witness flag in the transaction options.

I am not entirely sure if there are adverse consequences for this being enabled. but it does mean that we incorrectly say we want blocks with witness data when constructing getdata payloads (e.g. in the block puller), for instance:

[2018-11-08 01:43:41.0731 29] TRACE: Stratis.Bitcoin.P2P.Peer.NetworkPeer.AddSupportedOptions (inventoryType:MSG_BLOCK)
[2018-11-08 01:43:41.0731 29] TRACE: Stratis.Bitcoin.P2P.Peer.NetworkPeer.AddSupportedOptions (-):MSG_WITNESS_BLOCK

[dangershony]

Hmmm ok I will have a look at this, we are upgrading protocol version for PH so it might cause ripple effects, but we should actually not enable witness unless its activated

[zeptin]

Looking into this in more detail, the reason why the version is incorrectly set is actually due to the following:

ConnectionManager calls CreateNetworkPeerServer without specifying the version parameter:

NetworkPeerServer server = this.NetworkPeerFactory.CreateNetworkPeerServer(listen.Endpoint, this.ConnectionSettings.ExternalEndpoint);

NetworkPeerFactory.CreateNetworkPeerServer() instantiates a NetworkPeerServer which propagates the erroneous version into its Version property:

return new NetworkPeerServer(this.network, localEndPoint, externalEndPoint, version, this.loggerFactory, this, this.initialBlockDownloadState, this.connectionManagerSettings);

It looks like this affects all inbound peers via the following:

        private NetworkPeerConnectionParameters CreateNetworkPeerConnectionParameters()
        {
            IPEndPoint myExternal = this.ExternalEndpoint;
            NetworkPeerConnectionParameters param2 = this.InboundNetworkPeerConnectionParameters.Clone();
            param2.Version = this.Version;
            param2.AddressFrom = myExternal;
            return param2;
        }

I believe the transaction options causing witness requirements are still coming from the default setting in NetworkPeerConnectionParameters though.

[bokobza]

Hi @zeptin, can you trace "version" payloads and check that it is indeed the case that we send version as 70012?

[zeptin]

@bokobza Note that the protocol version actually gets used in several different payloads. But yes, it does. 7c1101 = 70012 when you flip the endian-ness and convert to decimal:

2018/11/12 10:51:07 PM [::ffff:191.235.x.x]:26178 IN  [version] -> 7c1101000900000000000000bde7e95b00000000010000000000000000000000000000000000ffffa9010dd806b8010000000000000000000000000000000000ffff7f00000166426a6c4acd3c25891d1453747261746973426974636f696e3a312e322e3380c2090001

2018/11/12 10:51:16 PM [::ffff:51.141.x.x]:26178 IN  [version] -> 7c1101000900000000000000c7e7e95b00000000010000000000000000000000000000000000ffffa9010dd806bd010000000000000000000000000000000000ffff7f00000166422111878521441f551453747261746973426974636f696e3a312e322e3380c2090001

2018/11/12 10:51:30 PM [::ffff:52.173.x.x]:26178 IN  [version] -> 7c1101000900000000000000d3e7e95b00000000010000000000000000000000000000000000ffffa9010dd806c8010000000000000000000000000000000000ffff7f0000016642f8479898c344e8691453747261746973426974636f696e3a312e322e3380c2090001

[zeptin]

(The above are some incoming connections from testnet. They all sent sendprovhdrs during handshaking, so they have presumably not applied 2733 or 2727 yet)

[zeptin]

Examining this further, there appears to currently be no impact having this flag set. The NetworkPeer code makes a distinction between preferred and supported transaction options, and the witness option flag is merely preferred.

The supported flags get assigned via the following in the NetworkPeer constructor, negating the effect of the witness flag being originally unconditionally set:

            this.preferredTransactionOptions = parameters.PreferredTransactionOptions;
            this.SupportedTransactionOptions = parameters.PreferredTransactionOptions & ~TransactionOptions.All;

While the flag should be correctly set depending on network and deployment status, this does not appear to be critical at this point.

[dangershony]

Probably the fix here should be to separate the services and make it per network (same like the refactor coming for NetwworkProtocol enum).