Disconnect network service bits 6 and 8 until Aug 1, 2018 #10982

TheBlueMatt commented at 7:53 PM on August 3, 2017: member

Immediately disconnect peers that use service bits 6 and 8 until August 1st, 2018 These bits have been used as a flag to indicate that a node is running incompatible consensus rules instead of changing the network magic, so we're stuck disconnecting based on the service bits, at least for a while.

Staying connected to nodes on other networks only prevents both sides from reaching consensus quickly, wastes network resources on both sides, etc.

Didn't add constants to protocol.h as the code there notes that "service bits should be allocated via the BIP process".

TheBlueMatt force-pushed on Aug 3, 2017

TheBlueMatt renamed this:
~~Disconnect network service bits 6 and 8 until Aug 1, 2018~~
Disconnect network service bits 8 until Aug 1, 2018
on Aug 3, 2017

TheBlueMatt commented at 10:23 PM on August 3, 2017: member

Updated to only disconnect bit 8 based on comments by the Bitcoin-ABC (bit 5 users) folks indicating they want to change their network magic to ensure good separation for their nodes. Based on the comments from the btc1 folks, I think we should consider merging this sooner rather than later.

TheBlueMatt force-pushed on Aug 3, 2017

TheBlueMatt commented at 10:36 PM on August 3, 2017: member

It was pointed out that irrespecitve of whether Bitcoin-ABC changes their network magic to further split their nodes off from nodes wasting their resources, disconnecting nodes with bit 5 will only help their nodes and future Core nodes, so I went ahead and re-added that check.

TheBlueMatt renamed this:
~~Disconnect network service bits 8 until Aug 1, 2018~~
Disconnect network service bits 5 and 8 until Aug 1, 2018
on Aug 3, 2017

jheathco commented at 12:01 AM on August 4, 2017: none

@TheBlueMatt any reason you started referring to bit 5 instead of bit 6? Looks like the PR still refers to bit 6 (as does master branch of ABC).

TheBlueMatt renamed this:
~~Disconnect network service bits 5 and 8 until Aug 1, 2018~~
Disconnect network service bits 6 and 8 until Aug 1, 2018
on Aug 4, 2017

TheBlueMatt commented at 12:10 AM on August 4, 2017: member

@jheathco heh, sorry, inconsistent 0base vs 1-base naming.

TheBlueMatt force-pushed on Aug 4, 2017

deadalnix commented at 8:01 PM on August 4, 2017: none

We'll be rolling out a new magic. Depending on how fast people upgrade, it'll take more or less time. Hard to give a specific timeline at this point in time.

Le 3 août 2017 21:55, "Matt Corallo" notifications@github.com a écrit :

Ideally we really wouldnt do this, does @jgarzik https://github.com/jgarzik or @deadalnix https://github.com/deadalnix have any update on using the network magic for their coins?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320073035, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0IaWnKhO94F_ZGsalSYRh4aOBbCRs8ks5sUiWfgaJpZM4Os6bg .

TheBlueMatt commented at 8:02 PM on August 4, 2017: member

@deadalnix awesome, sounds good. Note that its probably still a good idea for Core to do the disconnect-on-service-bit thing here as it just adds a second layer of ensuring the networks properly split and dont waste each others' resources.

luke-jr commented at 10:10 PM on August 4, 2017: member

I don't like the general idea since it burns a service bit unnecessarily, but since it's just for a year or so, and we don't seem to be in shortage of service bits, I don't mind it either.

fanquake added the label P2P on Aug 5, 2017

Leviathn commented at 2:45 AM on August 5, 2017: none

utACK

laanwj commented at 10:55 AM on August 5, 2017: member

Looks straightforward and correct to me - utACK https://github.com/bitcoin/bitcoin/pull/10982/commits/cae246e6d95bc1f3cbefe984dca3c100cd1a8743.

Would be good to create an issue to track this to make sure this code gets removed after T>=1533096000.

laanwj added this to the milestone 0.15.0 on Aug 5, 2017

jgarzik commented at 2:51 PM on August 5, 2017: contributor

NAK, this will cause a premature network split.

laanwj commented at 2:59 PM on August 5, 2017: member

One remaining relaying node is enough to prevent a premature chain split. Which should be fine as not everyone will be upgrading to 0.15 at the same time. If you think there's a risk of that, run a node without this code and make sure to be connected to both nodes signaling and not signaling this flag.

kek-coin commented at 3:09 PM on August 5, 2017: none

Since the S2X hardfork blockheight is known, would it be better to only disconnect when the block prior to the hardfork block is found and relayed?

jgarzik commented at 3:18 PM on August 5, 2017: contributor

Deploying this change for NODE_SEGWIT2X - bit 7 - creates chain splits in the wild on an inconsistent basis -- the upgrade rate to (0.15?).

This creates chain splits even though Bitcoin Core and segwit2x nodes are validating 100% the same rules today; it creates chain splits because of a presumed *future* rule deviation.

The outcome is a bunch of non-deterministic islands. This is a very hostile and unsafe change prior to segwit2x fork deployment.

Consider, for example, what would happen if this code were hypothetically deployed on July 1 2017 for NODE_BIP148. Nodes would be split off prematurely, at an inconsistent rate (rate of upgrade).

jgarzik commented at 3:29 PM on August 5, 2017: contributor

It is safe and a convenient optimization for this codebase to make this change after a chain split, but not before.

TheBlueMatt commented at 3:37 PM on August 5, 2017: member

@kek-coin we are absolutely not in the business of trying to determine what clients running incompatible consensus rules will do. Ignore @jgarzik's fudding, this obviously doesn't create a split until their incompatible rules kick in, it will only make upgraded nodes more cleanly separated, its not like the network wont still be well-connected.

jgarzik commented at 3:39 PM on August 5, 2017: contributor

Disconnecting service bits should only be enabled after peers have started banning each other on the network, thus proving the safety of service bit disconnect.

It is obviously unsafe to disconnect outside that set of conditions.

ETA: @TheBlueMatt Please reconsider that every disagreement of opinion is not "fudding", and we can have an honest debate.

jgarzik commented at 3:51 PM on August 5, 2017: contributor

Specific review:

The subject line says bits 6 and 8, yet the current code as of this writing masks bits 5 and 7.
bit 5 is NODE_BITCOIN_CASH = (1 << 5) in the BitcoinABC codebase, and bit 7 is NODE_SEGWIT2X = (1 << 7) in the btc1 codebase.
NODE_BITCOIN_CASH is provably divergent today, Bitcoin Core and BitcoinABC peers ban each other today, so it is obviously safe to add this disconnect logic as an optimization to what would automatically be arrived at already.
NODE_SEGWIT2X is not divergent today, Bitcoin Core and btc1 peers do not ban each other today, and including this bit in the disconnect logic would create new network disruptions that would not otherwise exist.

One bit is safe for the network as a whole, one bit is disruptive to the network as a whole - today.

jtimon commented at 4:00 PM on August 5, 2017: contributor

Who cares about those nodes not diverging today? Those sevice bits are used because you plan to diverge. We can start preparing today. If we set a date, what if you guys decide to change the date later? We cannot waste time trying to accommodate all the details of an underspecified, rushed ans underreviewed (because some review was rejected or ignored) implementation.

It is completely safe for all bitcoin nodes today and even for swx2 nodes, but even if it wasn't, I don't think we should spend any time trying to make things more sevure for swx2 nodes.

Concept ACK.

On 5 Aug 2017 6:51 pm, "Jeff Garzik" notifications@github.com wrote:

Specific review:

The subject line says bits 6 and 8, yet the current code as of this writing masks bits 5 and 7. 2.

bit 5 isNODE_BITCOIN_CASH = (1 << 5) in the BitcoinABC codebase, and bit 7 is NODE_SEGWIT2X = (1 << 7) in the btc1 codebase. 3.

NODE_BITCOIN_CASH is provably divergent today, Bitcoin Core and BitcoinABC peers ban each other today, so it is obviously safe to add this disconnect logic as an optimization to what would automatically be arrived at already. 4.

NODE_SEGWIT2X is not divergent today, Bitcoin Core and btc1 peers do not ban each other today, and including this bit in the disconnect logic would create new network disruptions that would not otherwise exist.

One bit is safe for the network as a whole, one bit is disruptive to the network as a whole - today.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320451104, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSlaKEYlSUSP2mJ38b48vZvj9pfByks5sVI-SgaJpZM4Os6bg .

sdaftuar commented at 4:05 PM on August 5, 2017: member

Concept ACK

It is safe and a convenient optimization for this codebase to make this change after a chain split, but not before. @jgarzik I believe it is safer for the network topology to not suddenly change at the same time of a consensus rule change -- that is likely to be disruptive (I could imagine users of our software could suddenly find themselves without any peers, depending on the adoption rate of btc1) and forces users to be reactive, rather than proactive, to networking issues that arise. We used similar reasoning with the peering changes that accompanied segwit: the preferential peering was active long before segwit activated, by design, so that the network topology could change gradually with software deployment, and any unexpected behaviors could be understood and corrected before the consensus changes took place.

I don't feel strongly about whether the change in this PR happens now (ie for the 0.15 release, which is imminent), or in a future 0.15.1 or so, assuming we expect the latter to happen well in advance of any planned chain splits. But I strongly believe the network topology should largely adjust pre-fork, and not post-fork, to minimize disruption.

Furthermore, I don't think that btc1 making a network magic code change at the fork point would eliminate the need for this PR -- the goal is for the network topology to start changing earlier, and not suddenly be disrupted. (Perhaps if btc1 changed network magic a few weeks or more before any planned fork, then that would be sufficient to eliminate the need here, but as I see https://github.com/btc1/bitcoin/issues/99#issuecomment-320450248 there is clearly no appetite for this.)

Based on the adoption rates of earlier releases, I don't expect the whole network to suddenly upgrade to 0.15 and cause a network split (I happen to personally run many older versions of bitcoin core, and I assume many others do too, which would serve as bridges).

If we had more time on our hands and someone was so inclined, a more conservative change would be to just limit the number of outbound peers that offer these service bits, eg to 2 out of our 8. However, given the relatively constrained time frames, I don't think the review burden of a more complicated change is worth it, given how low risk I view this change to already be. And if we want to get this merged for 0.15 or 0.15.1 the change should be as minimal as possible, along the lines of this PR.

Will test.

jgarzik commented at 4:06 PM on August 5, 2017: contributor

The specific and immediate impact of this change is that Bitcoin Core nodes would disconnect all segwit2x nodes, even though they share the same rulesets today.

TheBlueMatt commented at 4:10 PM on August 5, 2017: member

@jgarzik I'm really not sure what basis your argument has, here. This change very clearly wont harm 2x nodes as they already do preferential peering and will obviously stay well-connected thanks to the numerous 0.14 and previous nodes on the network, I'm really not sure how you could argue otherwise. The idea that the network should suddenly change topology come hard fork is only adding potential for issues on that day, so this change can only help y'all smoothly fork off.

NicolasDorier commented at 4:12 PM on August 5, 2017: contributor

nit: Would be nice to use named constants.

jgarzik commented at 4:12 PM on August 5, 2017: contributor

Disconnecting peers that are otherwise 100% interoperable is just bananas.

It is the opposite of the Robustness Principle ("be liberal in what you accept') of RFC1122 and elsewhere: https://tools.ietf.org/html/rfc1122#section-1.2.2

laanwj commented at 4:17 PM on August 5, 2017: member

nit: Would be nice to use named named constants.

I don't think that's necessary - the code is short-lived, and the comment already explains it well enough, and one could also say it's more self-contained like this. We wouldn't want to define these service bits with those that have been reserved by following the BIP process.

jgarzik commented at 4:52 PM on August 5, 2017: contributor

All,

Please strongly consider the signal it sends to the community, and other protocol/implementation developers, when changing Bitcoin Core to reject otherwise-working, otherwise-interoperable network clients.

This is akin to a Web server rejecting connections with a client string "Internet Explorer" while accepting client strings that include "Mozilla", but are otherwise 100% interoperable.

Let's figure out a way to cooperate and maximize interoperability. Let's be RFC 1122 compliant.

betawaffle commented at 4:59 PM on August 5, 2017: none

otherwise-working, otherwise-interoperable

If you see a car heading in your direction on the wrong side of the road, you don't wait until they have hit you to react.

gmaxwell commented at 5:02 PM on August 5, 2017: contributor

Disconnecting peers that are otherwise 100% interoperable is just bananas.

It is the opposite of the Robustness Principle ("be liberal in what you accept') of RFC1122 and elsewhere: https://tools.ietf.org/html/rfc1122#section-1.2.2 @jgarzik The the Postel maxim is widely, if not yet universally, considered to be a broken idea of the past. When it's brought up in the IETF it's usually with regret or with mocking-- it is directly responsible for many serious problems on the internet today. ( https://tools.ietf.org/html/rfc3117#section-4.5 http://programmingisterrible.com/post/42215715657/postels-principle-is-a-bad-idea ).

It's also frequently in direct conflict with security and reliability.

This is one of those cases. Sudden changes in network topology can leave a node isolated from compatible consensus rules. We depend on long lived connections for durability from various DOS attacks. If suddenly a node finds all its peers incompatible with it even if its smart enough to rapidly realize this and ban incompatible peers (as Bcash's adoption of the segwit signature scheme for replay protection provided but which nothing in s2x provides), it may find that it can not find anything to connect to, e.g. because attackers have filled most of the slots elsewhere on the network.

S2X nodes are not 100% compatible, because compatibility includes what the connection will do in the future as well.

A Bitcoin node depends on information being easy to spread and hard to stifle, but this assumption doesn't hold when you're surrounded with peers with incompatible rules-- the incompatible peers will stifle your communications with the Bitcoin network. The only way to be sure to have robust connections to peers with compatible rules when some event happens is to have them well before the event, so they enjoy protected high priority slots.

This was discussed extensively in the design and development of segwit, since segwit itself results in a change in P2P networking. After extensive consideration we found the best way to make sure there was no problem was to make the topology change in advance. In particular, this moves any topology change related issues for a single node to the time of upgrade, when the operator is certainly paying attention, rather than later when they are not.

Consider a node with 8 peers, all s2x nodes. At some height the s2x issuance activates, and s2x stops sending valid blocks to our node. Yet the s2x network then takes hours (like Bcash, or over a day like the btc1 testnet) to mine the even a single additional block, and because s2x has no replay protection invalid tx signatures will also not cause banning. When s2x does get a block it will only disconnect a single peer and may find connection slots exhausted all over the net (due to attacks or increased demand from other links cutting). If it has a single connection up to the Bitcoin network, if it has more blocks it may not even notice s2x blocks are invalid if they're part of a less-work chain, since s2x doesn't use the HF bit. All of this disruption, potentially quite severe and damaging (e.g. esp if our node in question is a Bitcoin miner) is avoided by not making a hard cut change to the network topology, but instead adopting a topology from the moment the node starts that will continue to be good for it in the future, with the change happening over time rather than as a network wide system-shock.

Above, the claim is made above that "Bitcoin Core nodes would disconnect all segwit2x nodes"; but that isn't the case-- only nodes running this newer software would do so. Historical update rates suggest that this will only be a moderate part of the network. But this moderate part is also important because it means there will be a reasonably large chunk of the network which are more likely to be healthy, partitioned, and more able to take the connections from nodes running older code which aren't aware of the topology change.

I'm not aware of, and I haven't seen presented any argument that there was an actual meaningful risk of any kind of premature partitioning as a result of this under any kind of realistic assumptions. If I've missed it, please walk me through how that might happen.

Let's figure out a way to cooperate and maximize interoperability.

That sounds great, but people have been trying to do so recommending basic prudent changes that would prevent disruption which your project has very vigorously (and in many causes insultingly) rejected.-- including things that even Bcash got right. Especially when your project insists on an nonnegotiable position of making these changes with many times less time than we have consistently expressed is needed to safely accomplish such things. If cooperation is in your plans, why is it not evidence in the timeframes or in the activities thus far?

Most confusingly, given the relative ratio of nodes in the network changes like this are likely to be even more protective of S2X users than they are of Bitcoin's users.

jtimon commented at 5:05 PM on August 5, 2017: contributor

Other implementations like libbitcoin used service bits core didn't and there was no problem. But your implementation with its service bit is designed to break compatibility. Why try to be compatible with something designed to be incompatible? As matt says, this will smooth your fork off.

On 5 Aug 2017 19:59, "Andrew Hodges" notifications@github.com wrote:

otherwise-working, otherwise-interoperable

If you see a car heading in your direction on the wrong side of the road, you don't wait until they have hit you to react.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320454768, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSoQZpKWVsyuzSCkyJa5a0-ZDysqNks5sVJ94gaJpZM4Os6bg .

TheBlueMatt force-pushed on Aug 5, 2017

TheBlueMatt commented at 5:40 PM on August 5, 2017: member

Fixed to use GetTime() instead of GetSystemTimeInSeconds, as I misread the comment and intended the mockable version, not that it matters much.

greenaddress commented at 5:49 PM on August 5, 2017: contributor

Concept ACK. Agree with @laanwj this needs to be tracked to remove the code once the disconnect is unnecessary.

Happy the code is confined and explicitly limited to do the disconnect until 1st of August 2018. I don't personally think in this case a constant is useful - comments are to the point IMHO.

morcos commented at 9:20 PM on August 5, 2017: member

Please strongly consider the signal it sends to the community, and other protocol/implementation developers, when changing Bitcoin Core to reject otherwise-working, otherwise-interoperable network clients. @jgarzik I can not agree more with this statement. However, it doesn't apply in the slightest to this situation. The btc1 client is already not interoperable with the Core client. It already contains code that will irreversibly split from Core in the very near future. All this PR is doing is easing the transition for both types of clients.

jgarzik commented at 11:49 PM on August 5, 2017: contributor

It is not easing a transition when fully interoperable clients are disconnected for no other reason than a service bit is set. For the time period starting now through segwit2x fork time, It is creating disruption where, provably, none would have otherwise existed.

Absent this change, Bitcoin Core and segwit2x peers will communicate without error for months to come.

jtimon commented at 2:37 AM on August 6, 2017: contributor

"Ease transition to incompatibility" makes no sense to me. Please @jgarzik explain why there's a problem in 0.15 nodes not being connected to swx2 nodes - preferrably with an example of something going wrong -, answer one or more of the question raised in this pr that you have ignored so far or please, please stop it.

Review: I think there's value in creating the constants even if the comments are clear because it reduces the chances of some other implementation (or even a programmer in the same implementation) to use a reserved bit without noticing. I propose the following constant names: SRV_BIT_INCOMPATIBILITY1 and SRV_BIT_INCOMPATIBILITY2. That will make clearer what these service bits are about while keeping them in the same place as other sevice bits for a code reader to read.

Btc1 review (don't ask me why I'm still doing it): One easy way to avoid getting disconnected from 0.15 nodes is not using any of those 2 incompatibility bits at all in your implementation. Another way would be to move to another bit, but then I would propose to create SRV_BIT_INCOMPATIBILITY3 (assuming nobody beats me to it).

Another much simpler way, which would also render this discussion useless (or so I think, I'm happy to be corrected and learn), would be for hardforks to always use the hardfork bit (you could even save the worry of whether the first hf block will have enough real txs to create a bigger block in an invalid way or miners will need to create artificial txs to fill the block with the needed amount of garbage).

<offtopic> I bet this comment will be ignored here by you, jef, like my suggestion to use the hf bit was ignored in the btc1 repository. I hope I'm wrong. I heard that it is better to be listen than to be listened. But I wonder if that still applies when you're talking to someone who never listens and just repeats the same thing over and over with different words or in a different order.

<super_offtopic> I once admired you. Last drop in a full glass.

On 6 Aug 2017 02:49, "Jeff Garzik" notifications@github.com wrote:

It is not easing a transition when fully interoperable clients are disconnected for no other reason than a service bit is set. For the time period starting now through segwit2x fork time, It is creating disruption where, provably, none would have otherwise existed.

Absent this change, Bitcoin Core and segwit2x peers will communicate without error for months to come.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320476760, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSprzP6qG3jluofMl2UI7xYxIcxChks5sVP-QgaJpZM4Os6bg .

btcdrak approved

btcdrak commented at 3:43 AM on August 6, 2017: contributor

tACK 469c549f76a8b9a490ba4a289405790718aca250

btcdrak commented at 4:04 AM on August 6, 2017: contributor

This change should be backported as well.

jonny1000 commented at 4:16 AM on August 6, 2017: none

ACK, based on user experience

Disconnecting peers that are otherwise 100% interoperable is just bananas. @jgarzik I disagree with you here. I was running the Bitcoin-ABC client before and after the recent chainsplit. Before the chainsplit occurred I was connected exclusively to many Bitcoin Core peers and was worried about what would happen after the split. Then when the chainsplit happened my Bitcoin-ABC node kept cycling through hundreds of Bitcoin Core peers and banning them. I was struggling to find good peers for days. The wallet was very difficult to use, it was often many blocks behind what people were telling me on internet chat forums and many of my transactions took a long time to propagate and I had to re-broadcast some of them hundreds of times for them to work. Please consider the actual user experience when making these decisions.

I do not understand what you want to happen. Are you expecting all nodes to be connected to each other, then in one instant when the chainsplit occurs, for two separate p2p networks to magically form? Sorry to be rude, but that seems "bananas" to me. It was tried with Bitcoin-ABC and I had a terrible experience.

Since nodes need to upgrade for SegWit2x anyway, why not build the new network magic before the chainsplit occurs, that way it can be far smoother than somehow expecting it to happen instantly.

gmaxwell commented at 8:15 AM on August 6, 2017: contributor

@jonny1000 And ABC had an easier time than S2X (as currently implemented, if anyone uses it) would because transaction relay post split from ABC triggered banning (gradually) due to the relay protection and eventually when the difficulty adjustment happened header relay did too.

S2X has no such reasonable protections. And things went better than they might have for ABC-- had there been more miners initially and they experienced connectivity like yours they easily could have split into little islands working against each other...

Moreover the churn from a sudden topology change would greatly amplify the effect of DOS attacks on the network, because part of our protections is that attackers have a hard time tampering with long uptime links with a history of good behavior. The sudden change turns that protection into a liability when peers with a history of good behavior all turn bad at once.

afk11 approved

afk11 commented at 8:20 AM on August 6, 2017: contributor

tACK 469c549

jl2012 commented at 2:38 PM on August 6, 2017: contributor

utACK 469c549

Disconnect network service bits 6 and 8 until Aug 1, 2018

These have been used to indicate incompatible consensus rules
instead of changing network magic, so we're stuck disconnecting them.

1de73f4e19

TheBlueMatt force-pushed on Aug 6, 2017

jl2012 approved

jl2012 commented at 5:59 PM on August 6, 2017: contributor

utACK 1de73f4

in src/net_processing.cpp:1267 in 1de73f4e19

1259 | @@ -1260,6 +1260,17 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1260 |              return false;
1261 |          }
1262 |  
1263 | +        if (nServices & ((1 << 7) | (1 << 5))) {
1264 | +            if (GetTime() < 1533096000) {
1265 | +                // Immediately disconnect peers that use service bits 6 or 8 until August 1st, 2018
1266 | +                // These bits have been used as a flag to indicate that a node is running incompatible
1267 | +                // consensus rules instead of changing the network magic, so we're stuck disconnecting

jnewbery commented at 9:02 PM on August 6, 2017:

nit: suggest change the wording of this comment to ... to indicate that a node is running incompatible consensus rules. We will disconnect based on these service bits until August 1st 2018 (when we expect the incompatible nodes to have changed their network magic).

We're not 'stuck' doing anything. This is strictly an improvement both for nodes which have hard-forked away and those which maintain Bitcoin consensus rules.

jnewbery commented at 9:05 PM on August 6, 2017: member

utACK

One nit to make the comment more precise. I agree with @NicolasDorier and @jtimon that we should use named constants for these service bits. I generally don't like magic numbers in code, and as Jorge points out, it'll make it less likely that someone else collides service bits by accident.

btcdrak commented at 9:44 PM on August 6, 2017: contributor

@jnewbery To bikeshed, using constants for the service bits does not help implementation wide coordination - that's something people are supposed to coordinate via the mailing list and in BIPs. But in this particular case the service bits were globbed by projects intending to be altcoins so I assume they didnt feel it necessary. I believe not using constants here is a strict improvement, and if anything there should be a mailing list post warning other implementations that these bits are apparently in use by incompatible software.

jtimon commented at 10:11 PM on August 6, 2017: contributor

How is not using constants a strict improvement? What's the downside of using constants? The mailing list seems completely orthogonal to using constants to me.

On 7 Aug 2017 00:44, "฿tcDrak" notifications@github.com wrote:

@jnewbery https://github.com/jnewbery To bikeshed, using constants for the service bits does not help implementation wide coordination - that's something people are supposed to coordinate via the mailing list and in BIPs. But in this particular case the service bits were globbed by projects intending to be altcoins so I assume they didnt feel it necessary. I believe not using constants here is a strict improvement, and if anything there should be a mailing list post warning other implementations that these bits are apparently in use by incompatible software.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320534830, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSgQyz4xhHHkzVvV2JkTpd59rPkxpks5sVjPJgaJpZM4Os6bg .

jnewbery commented at 10:16 PM on August 6, 2017: member

I also can't see how not using named constants is a 'strict improvement'.

The question is not whether or not the use of these service bits by other codebases is legitimate. The question is what makes our codebase clearer. Other possible values for nServices already have named constants. Why make these values an exception?

Not trying to make a political point - just my opinion on the clearest way to implement this for people who will read this code later.

But as you said, this is bikeshedding. ACK with or without.

TheBlueMatt commented at 12:12 AM on August 7, 2017: member

@jnewberry constants here would indicate the bits should be interpreted as some service provided, but because they were neither coordinated through the BIP process, nor are they in the experimental range and the bitcoin-dev mailing list informed, it would be incorrect to do so. Instead, they should be treated as only "first bit to disconnect" and "second bit to disconnect", which dont really need to be used elsewhere so there is little reason to add more diff for constants.

jtimon commented at 1:08 AM on August 7, 2017: contributor

I don't think they would indicate that, specially if they are named along the lines I propose (or even DISCONNECT_BIT1, DISCONNECT_BIT2 or whatever, they can be as explicit as you want.

I agree it's just a nit and not a big deal, but I really don't understand the opposition to the constants, nor that the "wrong process" argument is brought up again. As @jnewberry says this has nothing to do with processes and the mailing list: the constants would merely improve the readability of our code. I don't understand why would anybody be opposed to that.

On 7 Aug 2017 03:13, "Matt Corallo" notifications@github.com wrote: @JNewberry https://github.com/jnewberry constants here would indicate the bits should be interpreted as some service provided, but because they were neither coordinated through the BIP process, nor are they in the experimental range and the bitcoin-dev mailing list informed, it would be incorrect to do so. Instead, they should be treated as only "first bit to disconnect" and "second bit to disconnect", which dont really need to be used elsewhere so there is little reason to add more diff for constants.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10982#issuecomment-320542015, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSpu_FEOSB-CEXmnS0PpZU6IYsCI4ks5sVlaYgaJpZM4Os6bg .

NicolasDorier commented at 6:06 AM on August 7, 2017: contributor

if the problem is using a constant project wise, you can use a function scoped constant. It would improve readability while not making it appears as a coordinated service bit.

Making the code harder to read because some process was not followed is not a good excuse.

Anyway utACK even without it.

laanwj merged this on Aug 7, 2017

laanwj closed this on Aug 7, 2017

laanwj referenced this in commit c8b62c7de3 on Aug 7, 2017

jnewbery commented at 1:51 PM on August 7, 2017: member

constants here would indicate the bits should be interpreted as some service provided, but because they were neither coordinated through the BIP process, nor are they in the experimental range and the bitcoin-dev mailing list informed, it would be incorrect to do so.

SERVICE_NONE, SERVICE_NETWORK and SERVICE_XTHIN are constants, but are not defined in BIPs.

little reason to add more diff for constants.

There's always reason to add more diff to make the code clearer and more self-consistent.

I'm not as familiar with this code as you are, so I like things to be made as clear as possible. Having magic bitshift comparisons in the code seems unclear to me (especially when the code compares bits 5/7 and the comment below talks about bits 6/8).

gandrewstone commented at 2:19 PM on August 7, 2017: none

To avoid a premature network split, this change will simply force btc1 to not set the service bit. If you check the subver, you'll force btc1 to spoof your subver.

jameshilliard commented at 9:25 PM on August 7, 2017: contributor

@gandrewstone btc1 nodes should not be making outbound connections to nodes that don't support their consensus rules anyways, they should instead preferentially peer with each other. Inbound connections from pre-0.15.0 nodes will be enough to prevent a network fork from happening before they switch to their incompatible rules.

dagurval referenced this in commit f8bcb816eb on Aug 7, 2017

petertodd commented at 5:09 AM on August 8, 2017: contributor

Concept ACK

SamouraiDev commented at 5:54 AM on August 8, 2017: none

ACK

notespace commented at 6:02 AM on August 8, 2017: none

Please correct me if I'm wrong, but assuming a large portion of NYA-signaling miners would actually start using the btc1 client, wouldn't this change end up starving out miners who wish to use Bitcoin Core 0.15.0 before the fork, as they wouldn't be able to get blocks as quickly as the btc1-only mining network? Or is every miner supposed to be using the FIBRE network anyways?

Also, as a Core user making transactions, wouldn't this mean that it becomes harder for transactions to find their way into blocks? Or are you hoping the un-upgraded 0.14.3 nodes pick up all the slack of relaying these transactions, and when the old node count dwindles as people upgrade, makes the problem worse?

What prevents the btc1 project from just changing or disabling its service bit to avoid this disruption, like what @gandrewstone is saying?

As long as you are putting in specific rules for specific clients, why not just partition off at the fork block? Or slowly disconnect up to the fork, not all at once on upgrade?

automated-response commented at 6:17 AM on August 8, 2017: none

Why the other implementation wouldn't have just done the reciprocal of this by default at launch is beyond me. This will markedly improve connectivity between compatible peers both respective chains. Looks good.

gmaxwell commented at 7:19 AM on August 8, 2017: contributor

why not just partition off at the fork block?

Please read the explanations by Suhas and myself earlier. This was covered.

notespace commented at 7:58 AM on August 8, 2017: none

OK, I see that it makes sense not to immediately partition at the block.

What about gradually pushing out nodes using the peer selection logic? Then the network would gradually partition safely, even if all upgrade to run 0.15.0 nodes before the fork. Then there remains some cross-communication between btc1 nodes and core nodes.

CSmith7703 commented at 8:16 AM on August 8, 2017: none

This rashly merged commit exactly showed how corrupt the BCore Committee became and why the majority of Bitcoin community decided to go ahead with SegWit2X.

jonasschnelli commented at 8:21 AM on August 8, 2017: contributor

Can we properly define those bits in protocol.h?

jonasschnelli commented at 8:52 AM on August 8, 2017: contributor

Can we properly define those bits in protocol.h?

I also dislike the way how those bits where kidnapped (without the BIP process) but setting a proper constant is still the way to go (we have also done that for XTHIN).

Anyways. Concept ACK.

CSmith7703 commented at 8:55 AM on August 8, 2017: none

@jonasschnelli

kidnapped

Yeah, another negative word to defame dissenters. Such dirty tricks should be avoided.

Anyways, glad to see that you BCore committee tries to leave Bitcoin and fork your 2nd altcoin. Will the token still be called BUASF? I can't wait to sell it to buy more Bitcoin.

I3ck commented at 11:30 AM on August 8, 2017: none

I thought the #1 priority was to have as many nodes as possible?

piotrnar commented at 11:43 AM on August 8, 2017: none

I see some serious lack of imagination here, among the people pushing for this change.

Do you realize that when you start banning peers based on the content of their version messages, it will eventually make the content of these message obsolete?

With this change, you are breaking up the network protocol, by forcing certain nodes to stary lying about who they are and what they want.

gmaxwell commented at 11:52 AM on August 8, 2017: contributor

@piotrnar If they do that they will just harm themselves for the most part, as they connect to the thousands of bitcoin core nodes out there that won't tell them about the blockchain they want to hear about. Where is your concern that Bitcoin also disconnects litecoin nodes?

There will always be malicious peers out there that will lie. That we can't help. Also, this PR disconnects, rather than banning. (In fact, it protects the peers addresses from banning, which will avoid compatible software on the same IPs getting banned everywhere in the network... which happened to some ABC users-- where their Bitcoin node had a hard time getting connections because ABC had gotten them banned all over the place).

piotrnar commented at 12:09 PM on August 8, 2017: none

@gmaxwell, this is how the protocol evolves: different people introduce different features, that later get approved (or not) by the mining majority.

There is nothing wrong with this process. I don't understand why you have a problem with it.

Are you saying that you are not going to ever allow the miners to vote on any consensus protocol changes, other then the ones that the bitcoin core team has accepted?

CSmith7703 commented at 12:10 PM on August 8, 2017: none

@gmaxwell

If they do that they will just harm themselves

Lie. The 'They' Piotrnar talked about is that the people who run your malicious code to ban others. gMAXWELL, your dishonest ego is quite laughable.

There will always be malicious peers out there that will lie.

So why are you always the malicious one that tells lies frequently?

Pheromon commented at 12:17 PM on August 8, 2017: none

Are you saying that you are not going to ever allow the miners to vote on any consensus protocol changes, other then the ones that the bitcoin core team has accepted?

That's exactly what Blockstream is about: they want to be the only ones to decide how and when Bitcoin must evolve. They ignore the community and reject miners because they think they are bad for Bitcoin.

CSmith7703 commented at 12:20 PM on August 8, 2017: none

@Pheromon Exactly. In the ego of gMAXWELL, people who refuse it is anti-decentralization and anti-users. And people who say NO will be defamed with smear campaign.

piotrnar commented at 12:21 PM on August 8, 2017: none

They ignore the community and reject miners because they think they are bad for Bitcoin.

I don't know about "the community", but I must say that ignoring the miners can only end up very badly for this project. Doing so bitcoin core is digging its own hole.

CSmith7703 commented at 12:23 PM on August 8, 2017: none

@piotrnar In the ego of Blockstream, those eight to ten guys are "the community". People who are against those eight guys are anti-decentralization, anti-Bitcoin, anti-users, and liars.

gmaxwell commented at 12:23 PM on August 8, 2017: contributor

@piotrnar It sounds like you are expressing a misunderstanding of the Bitcoin system.

The consensus rules are what defines what is or isn't mining. Namecoin or BCash or whatever miners are not Bitcoin miners because they produce blocks which violate Bitcoin's rules. If you look at the Bitcoin whitepaper, section 8 paragraph 2 talks about how nodes are protected against being overpowered by a majority of hashpower mining blocks which are invalid because they enforce the rules themselves. It would have been possible to define Bitcoin a different way where nodes blindly trust miners, but that isn't how it works or has ever worked. A miner that breaks the rules is simply ignored, and this is an integral part of the system of incentives that keeps the system from becoming insecure.

This is all pretty far off-topic for this PR now... and it shows in that several of the last posts are nothing but random people (Pheromon, and CSmith7703) hurling abusive comments and insults at me. This is not appropriate here and not welcome. We don't come into your projects and insult you. Please take it elsewhere.

CSmith7703 commented at 12:25 PM on August 8, 2017: none

@gmaxwell

Half truth and actual lies again. piotrnar knowns Bitcoin, Gmaxwell knowns BCore. Since BCore is not Bitcoin, then it's fair for GMAXWELL and his blockstream buddies to ban other nodes.

Pheromon commented at 12:26 PM on August 8, 2017: none

I don't know about "the community", but I must say that ignoring the miners can only end up very badly for this project. Doing so bitcoin core is digging its own hole.

Exactly. And maybe that's their real aim: destroy Bitcoin to save fiat money and banks (like their main investor, AXA).

ghost commented at 12:26 PM on August 8, 2017: none

Please lock this.

Pheromon commented at 12:27 PM on August 8, 2017: none

If you look at the Bitcoin whitepaper, section 8 paragraph 2 talks about

If you look at the Bitcoin WP, you will not find a block limit as a consensus rule, also it's clearly stated that big (enormous!) blocks would be handled by miners.

So, it seems like you point at the WP only when you want.

piotrnar commented at 12:29 PM on August 8, 2017: none

The consensus rules are what defines what is or isn't mining.

Sure - no question about it.

However, what we should be asking is: who, how and why defines (and guards) the consensus rules?

CSmith7703 commented at 12:29 PM on August 8, 2017: none

@Pheromon Don't forget Gmaxwell looked upon Bitcoin WhitePaper and slandered Satoshi. @piotrnar The ego of Gmaxwell defines and guards the consensus rule. Blockstream has spent more than 40 Million in the past two years to form this definition.

Gmaxwell: the last posts are nothing but random people

Anyone who don't follow the ego of Gmaxwell are 'random people'.

jonasschnelli commented at 12:39 PM on August 8, 2017: contributor

Please stop political discussion immediately. Keep it technical.

CSmith7703 commented at 12:40 PM on August 8, 2017: none

@jonasschnelli This PR is purely political and out of evil will.

MarcoFalke commented at 12:41 PM on August 8, 2017: member

The review comments on pull requests are not meant for extended off-topic discussions. Please switch to another channel and keep this discussion for review comments on the code.

MarcoFalke locked this on Aug 8, 2017

MarcoFalke unlocked this on Aug 9, 2017

brynwaldwick commented at 5:01 PM on August 9, 2017: none

What is the rationale for this versus, say, building compatibility with nodes that use these service bits into the client?

Is there a plan for creating a version compatible with these nodes in the future? Is there any timeline for that?

If I'm running this client in production what is the recommended upgrade path in the case where only tiny shards of the network are not using these service bits?

betawaffle commented at 5:37 PM on August 9, 2017: none

@brynwaldwick Those nodes aren't and can't be made compatible. They are hard fork nodes.

MarcoFalke locked this on Aug 9, 2017