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

-bip148 option #10428

Closed
wants to merge 6 commits into from

Conversation

@earonesty
Copy link

commented May 19, 2017

Disabled by default, this optional parameter allows a user to choose bip148 enforcement.

@fanquake fanquake added the Consensus label May 19, 2017
@petertodd

This comment has been minimized.

Copy link
Contributor

commented May 19, 2017

Concept ACK

1 similar comment
@dcousens

This comment has been minimized.

Copy link
Contributor

commented May 19, 2017

Concept ACK

@achow101

This comment has been minimized.

Copy link
Member

commented May 19, 2017

utACK

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented May 19, 2017

Concept ACK

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented May 20, 2017

Does this mean that blocks from miners using Bitcoin Core generated block templates will be rejected by the same Bitcoin Core version running on users machines with this option?

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented May 20, 2017

@paveljanik No, since they signal segwit by default they will be accepted as long as they don't build on top of non-signalling blocks.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented May 20, 2017

Thank you for submitting this.

@Leviathn

This comment has been minimized.

Copy link

commented May 20, 2017

Has someone done a risk analysis on the use of this by a minority?

@earonesty

This comment has been minimized.

Copy link
Author

commented May 21, 2017

I found this to be a good summary of the risks of various fork scenarios: https://blog.blockchain.com/2016/02/26/a-brief-history-of-bitcoin-forks/

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

If this optional parameter is permitted, then hard fork optional parameters such as -bip102 should be permitted as well.

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

@jgarzik This is still a soft fork, the difference is just how it is activated. Activation is done by economic majority, the reason one may want to use an optional parameter is because measuring if the network has reached a suitable economic majority is not as straight forward as measuring signalling blocks, one should activate the flag once they are confident a suitable percentage of the economy intends to activate the flag. The fork is convergent as long as there is an economic majority or miner majority backing it since miners ultimately mine the most profitable chain. A hard fork like BIP102 however is never convergent.

@BenedictThompson

This comment has been minimized.

Copy link

commented May 21, 2017

Concept ACK

@chaosgrid

This comment has been minimized.

Copy link

commented May 21, 2017

If hash rate support is less than 50%, this option causes a hard fork - a contentious minority chain hard fork to be clear. This is risky and should not be allowed in the official client.

If you do allow this, you will need to allow other, less risky (only activate with 75% or more hash rate) hard fork proposals in options form as well!

@juscamarena

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

@chaosgrid no it doesn't, it causes a chain split in that case. If it becomes minority chain at the start it can still potentially reorg the legacy chain if miners jump ship. A hard fork on the other hand splits and users or miners running legacy software pre-HF will never follow the HF'd chain no matter how much hash power it has.

@rawodb

This comment has been minimized.

Copy link

commented May 21, 2017

Concept ACK for soft forks

@forkwar

This comment has been minimized.

Copy link

commented May 21, 2017

Concept ACK

@chaosgrid It should be noted that there is already precedence for giving users the ability to manually enable options that have the potential to cause a chain split. Using the invalidateblock command, for example, would also technically be a soft-fork that can be activated in a similar way to this proposal. These options are safe to include because they are off by default. No user will accidentally be out of consensus.

@earonesty

This comment has been minimized.

Copy link
Author

commented May 21, 2017

@forkwar All users that change default options without understanding what they do are at risk of being on another chain.

@earonesty earonesty closed this May 21, 2017
@earonesty earonesty reopened this May 21, 2017
@maaku

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

Shouldn't there be mechanisms in place to ensure direct connectivity to other bip148 nodes? Otherwise the network could become partitioned on Aug 1st.

@RHavar

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

Concept ACK, but the description should include a very strong warning of the risks involved. There is a lot of misinformation about bip148 (from both sides), including a lot of people being told it's the lower risk option. Without an appropriate warning I feel many people will be misled.

In fact, I'd strongly prefer the flag to be called "-very-dangerous-bip148" or something similar (which can also set precedent for other controversial like flags). If it is merged in without a similarly alarming prefix, it'll be taken as an endorsement. (e.g. from the reddit thread that is brigading this issue:)

This is important because many businesses support the UASF cause, however they only trust Core code. If it is in Core code, prepare for many businesses to support it.

@nopara73

This comment has been minimized.

Copy link

commented May 21, 2017

Most previous arguments here against UASF are based on false assumptions. However UASF is most likely going to be hugely disruptive. I expect wallets, exchanges and companies not implementing sufficient protections for chainsplit. Not even Core is working on it.

However it seems like the ginny is out of the bottle, the disruption is unavoidable, it's hard to tell how big it will be.
I was wondering if having a temporary (and already unavoidable) UASF disruption is a bigger risk, than crippling the Bitcoin development and the scaling of Bitcoin?

While the answer to that question can be vague, but more importantly we should decide when does not merging UASF by default becomes a bigger systemic risk than merging it?

@pdaian

This comment has been minimized.

Copy link

commented May 21, 2017

Concept NACK. This has high risk of leading to a consensus break and a chainsplit, and is a dangerous parameter as such. Not sure how all the usual suspects I see who generally argue against such are concept ACKing this...

@RHavar

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

Not sure how all the usual suspects I see who generally argue against such are concept ACKing this...

I am in general against bip148 for the reasons you state, however I don't have problem with the idea of adding support as an option. I guess it comes down to if you believe bitcoin core should allow people to choose dangerous options without forcing them to recompile.

Personally I think it should as long as it's very clear about the dangers and there isn't excessive maintenance cost in including it. However it's worth pointing out that this style of change is potentially a lot more disruptive than a simple controversial hard fork. (Not only may it lead to a prolonged chain split, it may lead to a wipe out of the original chain causing millions of dollars of loses)

@AllanDoensen

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@jameshilliard back to the code. This change allows for an 'eclipse' style attack as you have suggested. As such it does have to do with node counts. Again I repeat this change is dangerous and completely lacks community and miner support.

@mcgravier

This comment has been minimized.

Copy link

commented May 22, 2017

unlikely there will be a long coexistance of two chains

I believe, just "unlikely" isn't good enough for a 30bln dollar market.

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@AllanDoensen No, it does not do an eclipse attack like you seem to have incorrectly assumed. Bitcoin is already quite resistant to widespread eclipse attacks due mitigation measures in how peering is handled, they aren't really a major concern at this time.

@AllanDoensen

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@jameshilliard I do not believe I have incorrectly assumed anything with respect to this code. Motherhood statements will only get you so far, maybe you should spend more time writing & reviewing the actual code.

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@AllanDoensen The node count relation is explained under the "Does node count determine activation?" section here already. I'm not sure where you are seeing eclipse attack code as absolute node counts aren't really relevant to activation of BIP148. I'd suggest you read through the FAQ as it explains these issues already in detail.

@chaosgrid

This comment has been minimized.

Copy link

commented May 22, 2017

@earonesty

Indeed since user support is growing quickly with at least one minor exchange on board, it might be more dangerous not to. Maybe it's better not to presume an outcome

Currently, 40% of hash rate is signaling for emergent consensus block size control. Roughly 10% of nodes signal as well. "Maybe it's better not to presume an outcome" and also add an option for emergent-consensus block size or the revised BIP100?

@sgsqys

This comment has been minimized.

Copy link

commented May 22, 2017

unlikely here is nearly impossible.
like order of 1/2e256.

all Bitcoin basis is on
such value of probability theory.

we have bip149 in 2017 and bip148 in July
2018. segwit will activate before 2018aug,
no matter of hash rate power.

segwit is favorite of market (see Litecoin).

uasf chain will win with full confidence.
old chain or bu chain will be of Little
economic value.

@sgsqys

This comment has been minimized.

Copy link

commented May 22, 2017

in one word, segwit unlikely fail.
just like btc privkey unlikely be guessed.

@AllanDoensen

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@earonesty your code itself looks good. I mean no disrespect, but take issue with the concept not the implementation.

As for 'dangerous' or 'experimental', I find it somewhat hypocritical that there is support for this, but no support for doing similar for bip100/bip102 or EC which are arguably just as dangerous. Thus I would think it would be better if there was a general means of signalling support for bips (including segwit) rather then just the odd implementation for one off cases such as this. So I would think that your code should be made generic in parts to support a range of signalling for BIPS.

I also think that as there is a 'start date' for this, there should also be an 'end date' so that if it fails it is not active in the codebase forever. Would you consider an end date of 2 months(?) as a change to the code?

@AllanDoensen

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@jameshilliard I know you are referencing a pretty document with nice charts....but that is not what this code & this PR says. Please read the code and comment on the code rather then referencing a document that says what you think the code should be doing. You can read C++ yes?

@pstratem

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

unsubscribe....

@achow101

This comment has been minimized.

Copy link
Member

commented May 22, 2017

@AllanDoensen

then someone fires up a 10 thousand nodes on a VM and brings down the bitcoin network.

In what way would a lot of nodes "bring down the bitcoin network"? BIP 148 nodes will disconnect from (and ban IIRC) non-BIP 148 nodes after they relay a number of bip148-invalid blocks. non-BIP 148 nodes will still be able to connect to other non-BIP 148 nodes and maintain a separate non-BIP148 blockchain. Node's won't be crashing or doing anything that would "bring down the network".

There would be risk for transaction replay and for blockchain reorganizations. The miners are incentivised to use BIP 148 the BIP 148 chain will not be able to be wiped out by the non-BIP 148 chain. However the opposite of that is true; the non-BIP 148 chain could be wiped out by the BIP 148 chain if it were to be longer than it.

Anyways, you should read the "petty document" since it does explain a lot of this stuff.

@starrymoonlight

This comment has been minimized.

Copy link

commented May 22, 2017

@AllanDoensen one consideration should be the risk of not implementing the optional BIP148 flag. You are saying it is risky to add but have you considered the risk of not implementing it on the network if it were to activate in August? Again, this is only an option that is disabled by default. Core should consider all possibilities and the risk of not taking action in the event of a chain split IMHO is greater than the risk of adding an option that is disabled by default.

@AllanDoensen

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@achow101 yes the network would not go down, that was probably overreach, but many issues would arise as you have alluded to.

@rawodb

This comment has been minimized.

Copy link

commented May 22, 2017

@AllanDoensen please comment or review the actual code. I have doubt you have read the C++ code that is only 1 click away. As your statements and complaints have been false in relation to the pull request.

@nopara73

This comment has been minimized.

Copy link

commented May 22, 2017

@AllanDoensen I think what @starrymoonlight is talking about is not about systemic risk, but individual risk.
This all comes down to this. If you prefer Bitcoin Core to outsource the risk for the users then this optimal PR should be merged. If you prefer Bitcoin Core to protect the network, then this optimal PR should be closed and if UASF gains so much adoption that it becomes riskier for the network not to merge it, an emergency release should be issued with UASF enabled by default.

@maxweisspoker

This comment has been minimized.

Copy link

commented May 22, 2017

Concept NACK. This is more dangerous than it is being given credit for. What makes a soft fork "soft" is that non-upgraded nodes continue on the same chain and consider the new rules valid. Without 50% of hashing power behind it, a UASF will cause a chain split. Old nodes will be on the "wrong" chain, and new nodes will be on another chain.

The fact that much development and care has gone into segwit is NOT an argument for adopting it. That's a sunken cost fallacy. Not insignificant chunks of the miners, users, and economic actors seem to be opposed it. Their reasons and motivation is not relevant. All that matters is that segwit does not have near-universal acceptance, which has always been the standard for upgrading the protocol.

If you're going to merge in options that allow users to break off onto a different chain under not-unlikely scenarios, why not also add in non-segwit base blocksize increases as options too? Or how about a pow=sha3 switch? Heck, let's just make it a barebones non-descript PoW blockchain client and have the users set all the consensus parameters they want. Then their client will only connect to whatever chain they want to be on. I mean, if the consensus chain doesn't require unanimous consensus, why not? There's no harm if we're now claiming that converging onto one chain isn't important.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Concept NACK. BIP 148 carries high, unnecessary risk of a chain split, as has been discussed on the mailing list. This project has always held high standards for deploying consensus-changing proposals while minimizing risks to consensus splits. In my view, BIP 148 as drafted is incompatible with the goals of this project, and putting it behind a command line option does not address the fundamental nature of this incompatibility. (I'll refrain from posting further on BIP 148 as I agree that GitHub is not the best place for these discussions but as this is my primary objection to this PR I see no other choice but to make some mention of it here. I'll take further analysis of these concerns to the mailing list.)

[There are also technical issues with this PR -- such as the peering concerns raised by @maaku (which are completely correct), and the lack of inclusion of any tests for this code. These could be addressed, of course, but as a purely technical matter, this PR is not currently suitable for merging, even if BIP 148 were not flawed.]

@earonesty

This comment has been minimized.

Copy link
Author

commented May 22, 2017

@sdaftuar To my knowledge, this PR properly isolates bip148 nodes from other peers. Also, this PR is not "for bip148" - it is an effort to prevent bitcoin fragmenting and protect exchanges from the negative consequences of a reorg.

Right now 10% of the nodes on the network are running non-core UASF code - including SPV wallet nodes and some small exchanges. Failing to protect users and miners by providing an option that puts them on the side of the fork that's less likely to encounter a reorg seems irresponsible.

If the current trend continues, and we try to roll something out as a last minute reaction - it could be too late to protect our most important users. Having it as an option allows users to enable it if needed to side with whatever the majority may be. If it comes to that, we can enable it by default, and make an announcement that people should either recompile or enable the option. Or the reverse: remove it from the code base and announce it will no longer be supported.

Copy link
Member

left a comment

Concept ACK. Found some minor bugs.

There are circumstances where this won't upgrade cleanly from a non-BIP148 client: specifically, after July in the case where miners have produced invalid blocks, this will fail to reconsider them. However, this is an issue for all past softfork deployments (except BIP141) as well, so probably shouldn't be a blocker (and can be improved in a later PR).

@@ -99,5 +100,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const
ss << ")";
}
ss << "/";
if (!fBaseNameOnly && IsArgSet("-bip148"))
ss << "UASF-Segwit:0.3(BIP148)/";

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 22, 2017

Member

IMO it would be more technically-correct to simply add "BIP148 Enforcing" to the uacomments by default, when this flag is set.

if (IsArgSet("-bip148")) {
// BIP148 mandatory segwit signalling.
int64_t nMedianTimePast = pindex->GetMedianTimePast();
if ( (nMedianTimePast >= 1501545600) && // Tue 01 Aug 2017 00:00:00 UTC

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 22, 2017

Member

@paveljanik The date is already set, deployed, and cannot be changed now. Folk in CZ will simply need to upgrade further in advance, if the timing is less than ideal.

@@ -99,5 +100,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const
ss << ")";
}
ss << "/";
if (!fBaseNameOnly && IsArgSet("-bip148"))

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 22, 2017

Member

The IsArgSet is wrong. It should be GetBoolArg instead. (also below)

@@ -482,6 +482,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-blockmaxsize=<n>", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_SIZE));
strUsage += HelpMessageOpt("-blockprioritysize=<n>", strprintf(_("Set maximum size of high-priority/low-fee transactions in bytes (default: %d)"), DEFAULT_BLOCK_PRIORITY_SIZE));
strUsage += HelpMessageOpt("-blockmintxfee=<amt>", strprintf(_("Set lowest fee rate (in %s/kB) for transactions to be included in block creation. (default: %s)"), CURRENCY_UNIT, FormatMoney(DEFAULT_BLOCK_MIN_TX_FEE)));
strUsage += HelpMessageOpt("-bip148", "Enable BIP148/UASF");

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 22, 2017

Member

Should specify the default value.

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Shouldn't there be mechanisms in place to ensure direct connectivity to other bip148 nodes? Otherwise the network could become partitioned on Aug 1st.

@maaku This is no different from any other softfork. Unlike Segwit, there isn't a stripped block to be avoided. Perhaps some mechanism like this could be an improvement, but it probably isn't necessary, and can't be secure against sybil attacks anyway.

@RHavar BIP148 is indeed truly the lower-risk option.

While the answer to that question can be vague, but more importantly we should decide when does not merging UASF by default becomes a bigger systemic risk than merging it?

@nopara73 It is already a bigger risk to not merge BIP148, than to merge it.

This can be implemented on top of -blocknotify and RPC invalidateblock, and useragent change without this dangerous modification. Any volunteer to contribute this solution?

@paveljanik Why should Core require an external script/program to remain a fully validating node?

Correct me if I'm wrong, but in case of long coexistance of two chains, and then BIP148 chain overpowering the old one, wouldn't it cause very long reorg?

@mcgravier Only for non-full nodes (ie, failing to enforce BIP148). This is always and already a risk if miners create long invalid blockchains, it isn't a new thing introduced by BIP148 specifically. It is normal in a softfork that old nodes have this risk.

@jameshilliard back to the code. This change allows for an 'eclipse' style attack as you have suggested. As such it does have to do with node counts.

@AllanDoensen What are you talking about? The code doesn't do anything you describe. Why do you pretend to read the code, when what you're saying has nothing to do with what the code actually does??

BIP 148 carries high, unnecessary risk of a chain split, as has been discussed on the mailing list.

@sdaftuar As with any softfork, miners who create long invalid blockchains violating the rules can cause a chain split. Such a chain split is not caused by BIP 148, but by non-compliant miners creating invalid blocks. Furthermore, this risk is strictly reduced by increased adoption of the softfork, since it becomes less economically viable for miners to go on producing these invalid blocks very long.

@earonesty This PR should be closed and rebased against master.

@hugohn

This comment has been minimized.

Copy link

commented May 22, 2017

@earonesty: Failing to protect users and miners by providing an option that puts them on the side of the fork that's less likely to encounter a reorg seems irresponsible. If the current trend continues, and we try to roll something out as a last minute reaction - it could be too late to protect our most important users.

@luke-jr : It is already a bigger risk to not merge BIP148, than to merge it.

This kind of argument is weak I'm amazed it's been repeated so many times.

The simple existence of a soft fork patch that has the potential to wipe out the legacy chain does not warrant the inclusion of that patch in the official Core software, which must maintain a high standard for changes, as @sdaftuar and others have pointed out.

If say, BU releases a similar soft fork patch that also endangers the legacy chain in the event that they get 51% hash rate (in fact, they would be in a better position to do so), would you guys be OK with including that software patch into the Core client? No.

You know who else used similar logic / scare tactic to justify their action? the people that caused the Cold War nuclear arms race.
"If you don't improve our nuclear capability -> the Russians might nuke us first."
"If you don't include this patch in Core client -> the network might blow up."

TL;DR: This is the wrong sort of argument to be used for including changes in Core.

Erik Aronesty
@earonesty earonesty force-pushed the earonesty:0.14 branch to 5459079 May 22, 2017
@earonesty

This comment has been minimized.

Copy link
Author

commented May 22, 2017

OK, I was working off the UASF branch, I'm closing this and reopening rebased to master.

@earonesty earonesty referenced this pull request May 22, 2017
@earonesty earonesty closed this May 22, 2017
@earonesty earonesty deleted the earonesty:0.14 branch May 22, 2017
@webcaetano

This comment has been minimized.

Copy link

commented May 22, 2017

Moved to #10442

@earonesty earonesty referenced this pull request Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.