add a -bip148 option #10442

earonesty commented at 8:06 PM on May 22, 2017: none

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 commented at 8:11 PM on May 22, 2017: contributor

utACK

in doc/man/bitcoin-qt.1:456 in abbcfcd8b2 outdated

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

paveljanik commented at 8:16 PM on May 22, 2017:

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

earonesty commented at 8:28 PM on May 22, 2017:

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.

in src/init.cpp:497 in abbcfcd8b2 outdated

 493 | @@ -494,6 +494,7 @@ std::string HelpMessage(HelpMessageMode mode)
 494 |      strUsage += HelpMessageOpt("-blockmaxweight=<n>", strprintf(_("Set maximum BIP141 block weight (default: %d)"), DEFAULT_BLOCK_MAX_WEIGHT));
 495 |      strUsage += HelpMessageOpt("-blockmaxsize=<n>", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_SIZE));
 496 |      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)));
 497 | +    strUsage += HelpMessageOpt("-bip148", strprintf(_("Enable BIP148/UASF (default: %d)"),DEFAULT_BIP148));

paveljanik commented at 8:16 PM on May 22, 2017:

SPC before DEFAULT_BIP148 please.

in src/validation.cpp:1569 in abbcfcd8b2 outdated

1565 | @@ -1566,6 +1566,22 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
1566 |          flags |= SCRIPT_VERIFY_NULLDUMMY;
1567 |      }
1568 |  
1569 | +    if (GetBoolArg("-bip148",DEFAULT_BIP148)) {

paveljanik commented at 8:17 PM on May 22, 2017:

SPC before DEFAULT_BIP148 please.

paveljanik changes_requested

paveljanik commented at 8:18 PM on May 22, 2017: contributor

my NACK still holds though - see previous PR.

rawodb commented at 8:29 PM on May 22, 2017: none

utACK

nopara73 commented at 8:42 PM on May 22, 2017: none

For the record, previous discussions: #10417, https://github.com/bitcoin/bitcoin/pull/10428

ABISprotocol commented at 9:14 PM on May 22, 2017: none

utACK

AllanDoensen commented at 9:53 PM on May 22, 2017: contributor

NACK

mkwia commented at 10:07 PM on May 22, 2017: none

utACK

in src/validation.cpp:1569 in b360fd09c3 outdated

1565 | @@ -1566,6 +1566,22 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
1566 |          flags |= SCRIPT_VERIFY_NULLDUMMY;
1567 |      }
1568 |  
1569 | +    if (GetBoolArg("-bip148", DEFAULT_BIP148)) {

jtimon commented at 10:12 PM on May 22, 2017:

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

earonesty commented at 11:50 PM on May 22, 2017:

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

luke-jr commented at 1:42 AM on May 23, 2017:

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.

earonesty commented at 2:10 AM on May 23, 2017:

Cool ok

in src/validation.cpp:1580 in b360fd09c3 outdated

1575 | +                 !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus())) )   // and is not active.
1576 | +        {
1577 | +            bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
1578 | +            bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
1579 | +            if (!(fVersionBits && fSegbit)) {
1580 | +                return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit, please upgrade"), REJECT_INVALID, "bad-no-segwit");

jtimon commented at 10:15 PM on May 22, 2017:

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

earonesty commented at 11:47 PM on May 22, 2017:

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

in src/validation.h:451 in b360fd09c3 outdated

 446 | @@ -445,6 +447,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
 447 |  /** Check whether witness commitments are required for block. */
 448 |  bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);
 449 |  
 450 | +/** Check if Segregated Witness is Locked In */
 451 | +bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params);

jtimon commented at 10:16 PM on May 22, 2017:

Can't this be static?

EDIT: or preferably inlined

earonesty commented at 11:46 PM on May 22, 2017:

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

fanquake added the label Consensus on May 23, 2017

earonesty force-pushed on May 23, 2017

in src/validation.cpp:1581 in 5288ddab0f outdated

1576 | +                 !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus())) )   // and is not active.
1577 | +        {
1578 | +            bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
1579 | +            bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
1580 | +            if (!(fVersionBits && fSegbit)) {
1581 | +                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");

PRabahy commented at 3:21 AM on May 23, 2017:

Why version 0.13.1?

webcaetano commented at 3:28 AM on May 23, 2017:

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

earonesty commented at 12:37 PM on May 23, 2017:

Yeah

mkwia commented at 12:39 PM on May 23, 2017:

Perhaps "Bitcoin Core version ..."

earonesty commented at 12:42 PM on May 23, 2017:

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.

jtimon commented at 1:21 PM on May 23, 2017:

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

earonesty commented at 1:42 PM on May 23, 2017:

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

luke-jr commented at 1:54 PM on May 23, 2017:

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

hsjoberg commented at 12:10 PM on May 23, 2017: contributor

utACK

forkwar commented at 12:55 PM on May 23, 2017: none

utACK

in src/validation.cpp:1581 in 91722a0342 outdated

1577 | @@ -1578,7 +1578,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
1578 |              bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
1579 |              bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
1580 |              if (!(fVersionBits && fSegbit)) {
1581 | -                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");
1582 | +                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");

paveljanik commented at 3:17 PM on May 23, 2017:

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?

achow101 commented at 3:20 PM on May 23, 2017:

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

webcaetano commented at 3:28 PM on May 23, 2017:

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

hsjoberg commented at 9:48 AM on May 24, 2017: 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 force-pushed on May 24, 2017

earonesty closed this on May 25, 2017

earonesty force-pushed on May 25, 2017

earonesty commented at 11:07 PM on May 25, 2017: none

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

gmaxwell commented at 11:21 PM on May 25, 2017: 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.

add a -bip148 option 0f17665ac3

earonesty commented at 12:30 AM on May 26, 2017: none

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

earonesty reopened this on May 26, 2017

ajtowns commented at 5:55 AM on May 26, 2017: member

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 commented at 2:30 PM on May 26, 2017: member

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.

in src/validation.cpp:1580 in 0f17665ac3 outdated

1575 | +                 !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus())) )   // and is not active.
1576 | +        {
1577 | +            bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
1578 | +            bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
1579 | +            if (!(fVersionBits && fSegbit)) {
1580 | +                return state.DoS(0, error("ConnectBlock(): relayed block must signal for segwit"), REJECT_INVALID, "bad-no-segwit");

ajtowns commented at 3:58 PM on May 26, 2017:

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.

luke-jr commented at 4:48 PM on May 26, 2017:

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.

ajtowns commented at 7:27 PM on May 26, 2017:

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.

Xekyo commented at 3:14 PM on May 27, 2017:

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?

paveljanik commented at 2:01 PM on May 29, 2017:

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

dooglus commented at 7:35 AM on May 30, 2017:

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).

ajtowns commented at 11:45 AM on May 30, 2017:

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.

ABISprotocol commented at 6:21 PM on May 31, 2017:

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.

bip148: relax DoS detection during UASF

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.

c9580ffff7

bip148: functional test

Basic functional test for behaviour with -bip148 flag.

a020b85d21

ajtowns commented at 7:14 AM on May 27, 2017: member

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: https://github.com/earonesty/bitcoin/pull/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...

bip148: ensure nodes are synced prior to bip148 date 22bf9c14e1

ghost commented at 4:04 PM on May 27, 2017: none

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 commented at 11:21 PM on May 29, 2017: 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 commented at 11:53 AM on May 30, 2017: member

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

No functional tests demonstrating the code behaves as expected.
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)
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 https://github.com/earonesty/bitcoin/pull/2 has been closed, so...

keis commented at 4:15 PM on May 30, 2017: none

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.

Merge pull request #2 from ajtowns/bip148

Functional test draft

7f92d07958

sdaftuar commented at 8:50 PM on May 30, 2017: 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 commented at 1:10 PM on May 31, 2017: member

@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 commented at 3:03 PM on May 31, 2017: contributor

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 commented at 3:47 PM on May 31, 2017: none

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

ABISprotocol commented at 5:05 PM on May 31, 2017: none

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:

Function test has been proposed as described at https://github.com/bitcoin/bitcoin/pull/10442/commits/a020b85d213079a36b5f2ae94820b44e08dbbe07 for behavior with -bip148 flag.
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 commented at 7:16 PM on May 31, 2017: contributor

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 commented at 10:46 PM on May 31, 2017: member

Concept ACK.

I've also prepared a rewritten branch (bip148) on my repo https://github.com/bitcoin/bitcoin/compare/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.

ABISprotocol commented at 11:13 PM on May 31, 2017: none

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 commented at 12:03 AM on June 1, 2017: contributor

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 commented at 4:21 AM on June 1, 2017: member

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

in doc/man/bitcoind.1:459 in 0f17665ac3 outdated

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

dispardon commented at 7:54 AM on June 1, 2017:

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.

eordano commented at 11:34 AM on June 6, 2017:

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

dispardon commented at 8:03 PM on June 8, 2017:

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 commented at 9:57 AM on June 1, 2017: member

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 commented at 1:00 AM on June 3, 2017: none

Why do you "return state.DoS(0," for all blocks, not just segwit invalid?

On Wed, May 31, 2017 at 6:47 PM, Luke Dashjr notifications@github.com wrote:

Concept ACK.

I've also prepared a rewritten branch (bip148) on my repo master...luke-jr:bip148 https://github.com/bitcoin/bitcoin/compare/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.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10442#issuecomment-305339598, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGUcxVIVlgKvHCa5UKjtdIIgsfq1iHks5r_e3lgaJpZM4Ni0t5 .

gmaxwell commented at 4:32 AM on August 10, 2017: contributor

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

MarcoFalke closed this on Aug 10, 2017

DrahtBot locked this on Sep 8, 2021