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

add a -bip148 option #10442

Closed
wants to merge 5 commits into from
Closed

add a -bip148 option #10442

wants to merge 5 commits into from

Conversation

earonesty
Copy link

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

This is rebased from #10428

Includes changes from code review/suggestions.

@jmcorgan
Copy link
Contributor

utACK

Copy link
Contributor

@paveljanik paveljanik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my NACK still holds though - see previous PR.

@@ -451,6 +451,9 @@ Set maximum size of high\-priority/low\-fee transactions in bytes
.IP
Set lowest fee rate (in BTC/kB) for transactions to be included in block
creation. (default: 0.00001)
\fB\-bip148\fR
.IP
Enable BIP148 enforcement (see http://www.uasf.co)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No https://? Do we want link to an external site? What if the contents of the site changes? What if the domain owner changes?

Copy link
Author

@earonesty earonesty May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, since the primary reason for including the flag would be for people who already know about it and were going to install it anyway, and at least think they know what they are doing, an external link is probably not needed.

src/init.cpp Outdated
@@ -494,6 +494,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-blockmaxweight=<n>", strprintf(_("Set maximum BIP141 block weight (default: %d)"), DEFAULT_BLOCK_MAX_WEIGHT));
strUsage += HelpMessageOpt("-blockmaxsize=<n>", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_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", strprintf(_("Enable BIP148/UASF (default: %d)"),DEFAULT_BIP148));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPC before DEFAULT_BIP148 please.

@@ -1566,6 +1566,22 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
flags |= SCRIPT_VERIFY_NULLDUMMY;
}

if (GetBoolArg("-bip148",DEFAULT_BIP148)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPC before DEFAULT_BIP148 please.

Copy link

@rawodb rawodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@nopara73
Copy link

For the record, previous discussions: #10417, #10428

@ABISprotocol
Copy link

utACK

@AllanDoensen
Copy link
Contributor

NACK

@webcaetano webcaetano mentioned this pull request May 22, 2017
@mkwia
Copy link

mkwia commented May 22, 2017

utACK

@@ -1566,6 +1566,22 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
flags |= SCRIPT_VERIFY_NULLDUMMY;
}

if (GetBoolArg("-bip148", DEFAULT_BIP148)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small details: you could use gArgs::GetBoolArg directly instead of the wrapper. You could also create a global that is set only in init.cpp

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the wrapper from util.h is more consistent with other uses in the code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because the code was just refactored to create gArgs. New code should probably use it, rather than the wrappers which exist to avoid touching the old code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool ok

bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
if (!(fVersionBits && fSegbit)) {
return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit, please upgrade"), REJECT_INVALID, "bad-no-segwit");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "please upgrade" is a very appropriate comment here. Upgrade to what?

Copy link
Author

@earonesty earonesty May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right? maybe, please upgrade to Bitcoin version 0.13.2 or greater?

src/validation.h Outdated
@@ -445,6 +447,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
/** Check whether witness commitments are required for block. */
bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);

/** Check if Segregated Witness is Locked In */
bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params);
Copy link
Contributor

@jtimon jtimon May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be static?

EDIT: or preferably inlined

Copy link
Author

@earonesty earonesty May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, static is more consistent with other uses in the file, so i'll use that

bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
if (!(fVersionBits && fSegbit)) {
return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit, please upgrade to Bitcoin version 0.13.1"), REJECT_INVALID, "bad-no-segwit");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why version 0.13.1?

Copy link

@webcaetano webcaetano May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PRabahy Maybe please upgrade to Bitcoin version >=0.13.1 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps "Bitcoin Core version ..."

Copy link
Author

@earonesty earonesty May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t like "core". Bitcoin has no RFC to refer to, so Bitcoin literally is defined by this reference implementation. I think it would be great to use the RFC process, once Bitcoin is more mature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the person reading the error already has "Bitcoin version >=0.13.1", no?

Copy link
Author

@earonesty earonesty May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they cannot have that. 0.13.1 signals segwit. (if (!(fVersionBits && fSegbit)) ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@earonesty Node software does not control the block version header. Just remove the "please upgrade" bit.

@hsjoberg
Copy link
Contributor

utACK

1 similar comment
@forkwar
Copy link

forkwar commented May 23, 2017

utACK

@@ -1578,7 +1578,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
if (!(fVersionBits && fSegbit)) {
return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit, please upgrade to Bitcoin version 0.13.1"), REJECT_INVALID, "bad-no-segwit");
return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit, please upgrade to Bitcoin version >= 0.13.1"), REJECT_INVALID, "bad-no-segwit");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough. The user has to add -bip148 in addition. But you do not know if he is running Bitcoin Core at all. So this is wrong by design.

Who do you think should be/is the reader of this message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message will only end up in the debug.log, so there shouldn't be anything asking someone to upgrade.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove the please upgrade to Bitcoin version >= 0.13.1 part.
Make it short like ConnectBlock(): relayed block must signal for segwit

@hsjoberg
Copy link
Contributor

Perhaps the PR should see a rebase to 1 commit.
The commit titles for newest two give no value and only works in context with the first commit.

@earonesty
Copy link
Author

earonesty commented May 25, 2017

Maybe i did something stupid. I'll reopen when I get to a laptop

@gmaxwell
Copy link
Contributor

@earonesty You overwrote your PR branch with master. Since the changes being PR were removed github won't let the PR stay open (as there is nothing to merge). I'm not sure if restoring the commits will fix it, you might have to open a new PR.

@earonesty
Copy link
Author

earonesty commented May 26, 2017

Yeah, I tried to pull/rebase, and did something wrong. Thx.

@earonesty earonesty reopened this May 26, 2017
@ajtowns
Copy link
Contributor

ajtowns commented May 26, 2017

utACK

I was concerned by the issue luke-jr pointed out in #10428: "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." But I think what will actually happen in the case of a chain split with only minority hashpower on BIP148, is if you restart bitcoind with -bip148 on, say, August 7th, is that you be highly unlikely to extend the chain you had, and eventually the initially shorter BIP148 chain will become longer than the non-BIP148 chain as at August-7th, and you'll reorg. This behaviour will get worse the longer a user waits after August 1st to enable the option; again assuming a chain split and only minority hashpower on BIP148. This doesn't seem too bad to me, though could be better.

This probably isn't good enough for mining the BIP148 chain in this scenario -- if you think BIP148 is going to win, but hasn't yet and has already started, enabling -bip148 will leave you mining on a new, post August-1st fork of the non-BIP148 chain, creating a new chain split, until the original BIP148 chain has caught up.

In the event of BIP148 failing, as far as I can see, users can just restart bitcoind without the option, and will reorg to the longer chain. That seems fine, and minimally painful, given the circumstances.

This seems like it should have some behavioural tests included before being deployed though, given it changes consensus behaviour...

@ajtowns
Copy link
Contributor

ajtowns commented May 26, 2017

Okay, I've had a go at writing some functional tests, and I think the "restart with -bip148 sometime after August 1st" case is worse than I'd hoped -- a peer that mines on top of the chain they were following will get banned by BIP148 peers for passing on invalid blocks, and thus won't realise when the real BIP148 chain has overtaken their broken chain (at least until the BIP148 chain has somehow become the most work chain). That seems unreasonably fragile to me; I'll see if I can make a patch to prevent it happening.

bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
if (!(fVersionBits && fSegbit)) {
return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit"), REJECT_INVALID, "bad-no-segwit");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state.DoS(0, ...) here isn't entirely sufficient -- a non-BIP148 node will also forward any block building on the non-segwit block which will give a "prev block not found" or "prev block invalid" error which have DoS levels of 10 and 100 respectively.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't encounter prev block not found, because we should never fetch a block until all the headers are in the db.

prev block invalid probably needs to be changed to 0 for softfork safety... this isn't BIP148-specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I'm seeing "missing prev block" messages despite having seen the previous block, not "prev block not found" errors. I changed the DoS level for "prev block invalid" to 0, but I'm still seeing some disconnections between my test peers though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the expected behavior of someone starting with -bip148 be that it rejected all non-segwit signaling blocks since the startdate? So a node that joins on Aug 7th with that flag should reorganize to the current -bip148 branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what if majority of nodes is forced to restart at some day in August (e.g. there can be major security issue)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restarting is OK; the chainstate persists between restarts. The issue Xekyo is referring to is about what happens if someone enables the -bip148 switch after the start date (1st August). This pull request could see them frozen on their very own 0-length fork forever (or at least until the BIP148 fork becomes the longest chain).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only until the BIP148 fork becomes longer than your frozen chain, though -- the non-BIP148 chain could still be extending faster than the BIP148 chain in the meantime.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue that @dooglus mentioned regarding where someone enables -bip148 switch after start date, presumably August 1, and (assuming) the user experiences a 0-length fork, basically they'd have to delay usage of that wallet until BIP148 becomes the longest chain, or more generally, until there is a winner (if it's not BIP148 they'd have to sweep into another wallet that is supporting winner code). This is not too hard to address, because you could set it with a date, where if someone is selecting the option after the date of August 1, 2017, then it would throw a warning. The warning text would be something to the effect of "Activating this option after Aug. 1, 2017 could result in delays in your being able to synchronize with network until the BIP148 fork becomes longer than your chain. Do you wish to proceed?" Or something of that nature.

The real issue is whether or not the proposal functions and passes as it was designed and intended and can be presented (merged) as an option (not default). It can be, as I think @maaku pointed out in discussion, by addressing certain edge cases this could be merged into core.

When enforcing an optional user-activated soft-fork, peers that forward
blocks that are valid except for the soft-fork rules are not making a
denial of service attack. This change relaxes the rules so that blocks
built on invalid blocks do not trigger DoS banning.
Basic functional test for behaviour with -bip148 flag.
@ajtowns
Copy link
Contributor

ajtowns commented May 27, 2017

I've made a pull request with a draft of a functional test and the "prev block invalid" DoS level changed to 0 when -bip148 is active: earonesty#2

I'm still getting less peer connectivity than I expect and I'm not really seeing why. For me, just enough peers remain connected for the entire network to achieve consensus, but that seems like it's due to luck...

@ghost
Copy link

ghost commented May 27, 2017

utACK

Looking upon the concerns of other reviewers, I'd prefer if there is a modification to the Title of the client or an identifying message that informs that the user is indeed signalling towards BIP148, I can see scripts that could intentionally add this flag without the user knowing (although really-highly-unlikely)

@nomnombtc
Copy link
Contributor

NACK for now. This seems dangerous if the whole ecosystem is not behind it - and it is very hard to measure, social media can be easily manipulated. Some people will enable it and potentially lose money.

But I would actually support it if other controversial BIP's also would be available via optional commandline switch, like BIP10{0,1,2,9}. Either give the user all available options or none.

@ajtowns
Copy link
Contributor

ajtowns commented May 30, 2017

Okay, I'm a NACK on this PR for the following reasons:

  1. No functional tests demonstrating the code behaves as expected.
  2. Liklihood of partitioning the p2p network -- if there a chain split, BIP148 nodes will disconnect non-BIP148 nodes due to the non-BIP148 chain being invalid, and even if the BIP148 chain eventually becomes longer, that the non-BIP148 nodes still have an invalid chain to try to pass on still seems to cause disconnection before consensus can be achieved. (This is probably recoverable via a single peer passing on the BIP148 chain between partitions, but it's still pretty poor)
  3. Unexpected behaviour when switching to -bip148 after August 1st if the most work chain doesn't support BIP148 -- you won't immediately switch to a BIP148 chain, instead you'll stall until on the non-BIP148 chain until the BIP148 chain has caught up with where you were. This interacts with the previous problem -- if you switch to BIP148 after August, from a non-BIP148 chain with more work, you'll try passing on your chain to BIP148 peers and get disconnected for your troubles.

I think those are solvable objections, but earonesty#2 has been closed, so...

@keis
Copy link

keis commented May 30, 2017

Would it make sense to add the inverse option as well considering this is a risky change. The inverse here would mean something like "reject all blocks after aug 1 that ARE signalling". This means the potential gigantic reorg is not a (as) big worry as you can optionally stay on the old chain.

@sdaftuar
Copy link
Member

Concept NACK: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014377.html

After the IRC discussion from Thursday (https://botbot.me/freenode/bitcoin-core-dev/msg/86145297/), I think this PR should be closed.

@ajtowns
Copy link
Contributor

ajtowns commented May 31, 2017

@keis The inverse is actually "reject the chain where ALL blocks after aug 1 are signalling" -- otherwise you'd get three chains: one where some signal and some don't (current), one where all signal (bip148), and one where none signal (your proposal). The easiest way to actually do that is to wait until Aug 1, take the first block on the bip148 chain and run "invalidateblock" on its hash, but you need to wait until Aug 1 to do that.

@maaku
Copy link
Contributor

maaku commented May 31, 2017

A UASF, no matter the specific proposal, is built entirely upon user choice: it forces activation because an economic majority of users choose to use it over the other chain, even if the other chain has more work behind it.

I believe it is the consensus of developers here that user choice is a critically important check on the ecosystem, and a vital part of maintaining the balance of power between various players:

  • Should "the developers" try to push an unpopular consensus rule change, user choice not to upgrade will prevent it;
  • Should industry try to mandate a change without widespread support, user choice not to upgrade it would prevent it;
  • Should miners force an unwanted miner-activated soft fork, proactive user action in invalidating that chain will prevent such a hostile fork; and
  • Should miners prevent miner-coordinated activation of otherwise popular and desired technology, the UASF forces the popular outcome in a timely manner.

User-activated soft forks, or at least the viable threat of them, is a critical check and balance in the game theory that makes bitcoin work. However it would be naive to believe that the economically significant majority of users have the capability to do much more than run their bitcoin wallets from the distribution channels already in place. In some cases it is ignorance -- not knowing how to build bitcoind, or not having a binary distribution pathway for their platform or environment. In other cases policy may prevent them from doing so -- maybe they are contractually obligated to “run Bitcoin Core” and lawyers need to get involved to decided if a UASF patch on top counts as a contractual breach.

For this reason I believe it is the responsibility of Bitcoin Core -- and other wallets -- to provide an activation flag for any and all UASF proposals that have achieved a significant level of community backing, at least to the extent that it is practical to do so, and to make such a change available through the normal distribution channels. Arguing what level of community backing is required that would trigger that obligation of support, and how to measure it is interesting, but irrelevant to this case. Wherever that line is, we’ve clearly crossed it already.

As maintainers of wallet software we all have an obligation to the bitcoin ecosystem to proactively take actions which preserve user choice and therefore prevent handicapping the game theory incentives that keep bitcoin decentralized, regardless of how we may or may not feel about each proposal.

Bitcoin Core should, as a matter of its responsibilities to the community, properly review the BIP-148 implementation, fix edge cases identified, and merge a BIP-148 flag (off by default). Bitcoin Core should continue to do the same for any UASF proposal that achieves a similar level of user mindshare and interest as BIP-148 does today. To not do so would be harmful to the long-term incentives of bitcoin.

@Kixunil
Copy link

Kixunil commented May 31, 2017

@ajtowns You'd have to wait until a fork occurs, not just until 1 August.

@ABISprotocol
Copy link

ABISprotocol commented May 31, 2017

At this point, I fully concur with the reasoning of @maaku as described here. The idea as described is simply to "fix edge cases identified, and merge a BIP-148 flag (off by default)."

Some notes:

  1. Function test has been proposed as described at a020b85 for behavior with -bip148 flag.

  2. At Travis, build passing, there is job canceled and a job failed at the time of this comment, see details at https://travis-ci.org/bitcoin/bitcoin/builds/237618035

With fixes as described by @maaku I support the proposal as basically elucidated here with the additional commits necessary to address edge cases, which should be minimal.

I also urge skeptics to be aware of the overall benefits of this proposal and to consider them rather than looking only at potential negatives or assuming danger. A balanced view is necessary in order to be able to approach this matter constructively.

Edit: ACK

@maaku
Copy link
Contributor

maaku commented May 31, 2017

If it is easy to do (I don't really know that part of the code) I would suggest a NODE_BIP148 service bit and try to maintain two peers with that bit set. If implementing this is non-trivial, I would not demand it however, since partitioned networks can be manually merged with a single bridge.

@luke-jr
Copy link
Member

luke-jr commented May 31, 2017

Concept ACK.

I've also prepared a rewritten branch (bip148) on my repo master...luke-jr:bip148

This branch does a few things:

  • Replaces the extra "UASF-Segwit:0.3(BIP148)/" version component with a simple "+BIP148" (or "!BIP148" if disabled) UA comment. This uses "+" or "!" characters which are forbidden by Core (but not BIP 14) in -uacomment options to avoid confused users setting up their node incorrectly.
  • Adds a NODE_BIP148 service bit used exclusively for ensuring at least some peers are also enforcing BIP148. Currently it uses temporary-experiments bit 27. I am uncertain if a permanent service bit should be assigned, since this will be useless once the risk of chain splitting is past.
  • Added logic to rewind the blockchain at startup when/if a user upgrades from a non-BIP148 node which had an invalid chain as its tip.
  • Replaced the conditional DoS banning on invalid prev-block. This will never ban for invalid prev-block, since that condition is possible for any softfork, and the cost of producing invalid blocks is prohibitively high enough to prevent trying to abuse this for DoS.

@earonesty Please review my branch and overwrite your own when satisfied.

Copy link

@ABISprotocol ABISprotocol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably address the issues which were also brought up earlier as a potential concern here.

Seems good if it ensured users would be synced prior to bip148 date (I am assuming Aug 1) and not after, otherwise would need some kind of warning text to come up for people trying to activate after Aug 1, as suggested in my previous comment on the issue.

@maaku
Copy link
Contributor

maaku commented Jun 1, 2017

Concept ACK using a temporary-experimental service bit. That is the correct approach here, I believe, as it is basically only needed from Aug 1st until resolution.

@ajtowns
Copy link
Contributor

ajtowns commented Jun 1, 2017

At first blush, luke-jr's proposed changes look like they should address all my concerns; haven't compiled and tested yet though.

@@ -456,6 +456,9 @@ Set maximum size of high\-priority/low\-fee transactions in bytes
.IP
Set lowest fee rate (in BTC/kB) for transactions to be included in block
creation. (default: 0.00001)
\fB\-bip148\fR
Copy link

@dispardon dispardon Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "-reorgdefencebip148"
Core has the responsibility of providing users with the safest software. If BIP148 has a risk of wiping out blocks accepted by core, then it is also possible for miners to maliciously steer core nodes away from BIP148 for 1 or 2 blocks (in the case where the legacy chain has been reorged onto BIP148). At this point it is irresponsible for core to not have a solution ready "please enable -reorgdefencebip148 flag" and instead only have the option of scrambling to write, test, and release an emergency patch and say "users please download this emergency release".

If we're going to have more UASFs, which seems like a common opinion, reorg protection has to be considered and supported by core.

Hopefully this puts the bip148 in core discussion into perspective.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 1 orphaned block happens about once a week
  • You are suggesting a completely different functionality
  • More info: BIP148

Copy link

@dispardon dispardon Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not suggesting different functionality. Sorry if I wasn't clear. What I am suggesting is that the best way to protect users from a reorg is by switching to a segwit signaling chain (This PR).

It's best to be in a situation where users only need to switch a simple flag rather than download a brand new (maybe rushed) release.

But some are fighting the idea of bip148 on bitcoind. So what I am suggesting is a simple name change to put things into perspective to avoid that argument.

The perspective being: Users need a flag to prevent reorg so activating bip148 isn't about agreeing with bip148 but instead to protect their node from reorg.

@ajtowns
Copy link
Contributor

ajtowns commented Jun 1, 2017

In src/net_processing.cpp:ProcessMessage the "missing prev block" check has an additional DoS test that eventually gets triggered if the invalid chain reaches 50 blocks:

            if (nodestate->nUnconnectingHeaders % MAX_UNCONNECTING_HEADERS == 0) {
                Misbehaving(pfrom->GetId(), 20);
            }

The "if" triggers every 10 blocks, and the Misbehaving() increments the ban score by 20 until it hits a default threshold of 100. So this should be enough to start partitioning the network if a UASF chain split lasts as little as 9 hours. This code was introduced in #8305 for context.

With the above code commented out, and the DoS score of "prev block invalid" changed from 100 to 0, the BIP148 and non-BIP148 nodes in my functional tests remain fully connected even if the chain split lasts for over 500 blocks.

@earonesty
Copy link
Author

earonesty commented Jun 3, 2017 via email

@earonesty earonesty mentioned this pull request Jun 5, 2017
@gmaxwell
Copy link
Contributor

BIP 148 was a big success. Thanks to all involved. I think this PR can be closed now.

@maflcko maflcko closed this Aug 10, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet