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

Implementation of BIP 101 : maximum block size increase #6341

Closed

Conversation

Projects
None yet
@gavinandresen
Copy link
Contributor

commented Jun 26, 2015

Transaction volume is approaching the one-megabyte blocksize limit. We will never reach that limit, because several things will happen as we get close:

  1. Transaction confirmation times for transactions with a given fee will rise; very-low-fee transactions will fail to get confirmed at all.
  2. Average transaction fee paid will rise
  3. People or applications unwilling or unable to pay the rising fees will stop submitting transactions
  4. People and businesses will shelve plans to use Bitcoin, stunting growth and adoption

That will be a major economic shift from the status-quo of low transaction fees. See the preliminary BIP100 proposal: http://gtf.org/garzik/bitcoin/BIP100-blocksizechangeproposal.pdf for more discussion.

There is rough (but not 100%) consensus that at some point fees will rise "high enough" that the 1MB blocksize limit will have to be raised, but no consensus on what that point should be and no consensus on any process to determine what that point should be.

BIP 101 and the changes in this pull request are intended to keep the economic status quo (and keep Bitcoin growing) while acknowledging the engineering reality that networks do not have infinite bandwidth.

I addressed technical objections in a series of blog posts at http://gavinandresen.ninja/time-to-roll-out-bigger-blocks .

Any consensus rule change should have thorough code review and time for testing, so I am submitting this code now so that it has thorough code review and testing before transaction volume gets closer to the one megabyte limit.

Higher-level discussions on the merits of BIP 101 versus other proposals belongs on the bitcoin-dev mailing list (which just moved: https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev )-- please limit the conversation here to discussion of the code changes, and rest assured these changes will not be pulled into Bitcoin Core unless there is consensus around BIP 101.

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2015

The per-message size sanity checking commit is also pull request #6261

The mocktime fixes are pull request #6337 : MERGED, commits removed.

The remove coinbase payment key from keypool commit will be obsolete if #5994 is merged : MERGED, commit removed.

@btcdrak

View changes

src/chainparams.cpp Outdated
// 1MB max blocks before 1 Aug 2015
// Then, if miner consensus: 8MB max, doubling every two years
nMaxSizePreFork = 1000*1000; // 1MB max pre-fork
nEarliestSizeForkTime = 1438387200; // 1 Aug 2015 00:00:00 UTC

This comment has been minimized.

Copy link
@btcdrak

btcdrak Jun 26, 2015

Member

There should be a nLatestSizeForkTime which is the latest the fork can be activated, and if not, the fork would fail.

@mjamin

This comment has been minimized.

Copy link

commented Jun 26, 2015

Higher-level discussions on the merits of BIP 101 versus other proposals belongs on the bitcoin-dev mailing list

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2015

Note to readers: in its current form there is a near zero chance of this getting merged due to a number of BIP-level issues in addition to debate about the patch itself. For instance, Gavin has never given any details about testing; at minimum we'd need a BIP16 style quality assurance document. We also frown on writing software with building expiration dates, let alone expiration dates that trigger non-deterministically. (Note how my recently merged CLTV considered the year 2038 problem to avoid needing a hard fork at that date)

Until these issues are addressed I an many other contributors will be muting this thread and ignoring comments until the BIP itself is fixed. Much of the discussion we see in conversations around this subject is highly repetitive and a big timesink; don't interpret silence as agreement.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 26, 2015

There is rough (but not 100%) consensus that at some point fees will rise "high enough" that the 1MB blocksize limit will have to be raised, but no consensus on what that point should be and no consensus on any process to determine what that point should be.

See the mailinglist for why I totally disagree with the fact that fear of higher fees are a reason to increase the block size. If fees rise high enough, and people pay them, that sounds like a fantastic problem to have...

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 26, 2015

I'm happy to see the code for review openly here, but definitely do not take the block size discussion here.

@theuni

View changes

src/chainparams.h Outdated
/** Maximum block size of a block with timestamp nBlockTimestamp */
int ActivateSizeForkMajority() const { return nActivateSizeForkMajority; }
uint64_t SizeForkGracePeriod() const { return nSizeForkGracePeriod; }
uint64_t MaxBlockSize(uint64_t nBlockTimestamp) const {

This comment has been minimized.

Copy link
@theuni

theuni Jun 26, 2015

Member

Could you please pass nSizeForkActivationTime in here, rather than changing the const assumption of Params()? main can be responsible for keeping tabs of the activation time.

It will make life much easier for libbitcoinconsensus. I should think that it would make it easier for testing as well, since you could more easily fake a time for a single test.

@theuni

View changes

src/main.cpp Outdated
@@ -1870,7 +1880,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin

nInputs += tx.vin.size();
nSigOps += GetLegacySigOpCount(tx);
if (nSigOps > MAX_BLOCK_SIGOPS)
if (nSigOps > Params().MaxBlockSigops(block.GetBlockTime()))

This comment has been minimized.

Copy link
@theuni

theuni Jun 26, 2015

Member

Please use chainparams here rather than Params().

@theuni

View changes

src/main.cpp Outdated
@@ -1886,7 +1896,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
// this is to prevent a "rogue miner" from creating
// an incredibly-expensive-to-validate block.
nSigOps += GetP2SHSigOpCount(tx, view);
if (nSigOps > MAX_BLOCK_SIGOPS)
if (nSigOps > Params().MaxBlockSigops(block.GetBlockTime()))

This comment has been minimized.

Copy link
@theuni

theuni Jun 26, 2015

Member

Here too, and wherever else it applies. I'll avoid pointing out any others.

@theuni

This comment has been minimized.

Copy link
Member

commented Jun 26, 2015

Only had a minute to look over it, but I added a few comments about code for the sake of reviewing code. Policies and numbers can be discussed elsewhere, it's reasonable to critique the code independent of those discussions.

I'll give it a thorough review over the weekend.

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2015

I'm going to delete a few comments from @btcdrak and @rebroad that don't follow the "comment on the code-- high-level discussion goes to bitcoin-dev mailing list"

@theuni : thanks for the review-- this was forward-ported from 0.10 to 0.11 to git HEAD. It would be helpful if there was a design document for libbitcoinconsensus that I could code against...

@theuni

This comment has been minimized.

Copy link
Member

commented Jun 26, 2015

@gavinandresen Point taken. I'm not sure if we're quite to that point yet, though. We're still in the "move things around so we can do something useful" part and not quite to the "what useful things to do and how" part.

In particular for this PR, consensus functions need to work statelessly, so something like CheckTransaction() should have deterministic results depending only on its parameters and chainparam constants. That is a work-in-progress atm, see #6051 as an example of the current momentum. FWIW, that's why we've started passing chainparams and consensusparams around everywhere.

For the scope of this PR i don't think it's necessary to worry about that too much, but it'd be nice if it didn't make chainparams itself stateful.

That said, these new vars/functions probably belong in consensus/params.h rather than chainparams anyway; that's the new home for consensus-critical stuff.

I'd prefer to see something like:

diff --git a/src/consensus/params.h b/src/consensus/params.h
index c480a1c..b4b97f6 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -24,7 +24,22 @@ struct Params {
     bool fPowAllowMinDifficultyBlocks;
     int64_t nPowTargetSpacing;
     int64_t nPowTargetTimespan;
+
+    /** Maximum block size parameters */
+    uint32_t nMaxSizePreFork;
+    uint64_t nEarliestSizeForkTime;
+    uint32_t nSizeDoubleEpoch;
+    uint64_t nMaxSizeBase;
+    uint8_t nMaxSizeDoublings;
+    int nActivateSizeForkMajority;
+    uint64_t nSizeForkGracePeriod;
+
     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
+    uint64_t MaxTransactionSize(uint64_t nBlockTimestamp, uint64_t nForkTime) const { ... };
+    uint64_t MaxBlockSize(uint64_t nBlockTimestamp, uint64_t nForkTime) const { ... };
+    uint64_t MaxBlockSigops(uint64_t nBlockTimestamp, uint64_t nForkTime) const { return MaxBlockSize(nBlockTimestamp, nForkTime)/50; }
+
+    }
 };
 } // namespace Consensus
@sandakersmann

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2015

concept ACK

@gavinandresen gavinandresen force-pushed the gavinandresen:blocksize_fork_0.12 branch Jun 29, 2015

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

Reworked to fix @theuni code review comments:

  • Made chainparams constant: main.cpp holds a sizeForkTime global (that behaves like a std::atomic)
  • Moved block size fork params/methods to consensus/params.h

Regarding your comment on how peers that send over-maximum-blocks are handled:

That is a behavior change, but only if you send blocks between 1 and 2 megabytes before the fork switchover. Before: Send a block message > 2 MB: you get immediately disconnected (but not banned). Between 1 and 2MB: CheckBlock() would call Misbehaving() and you might get disconnected/banned (if not whitelisted).

With this pull request: any oversize block will get you immediately disconnected.

I started to make the behavior more consistent... but it's complicated because Misbehaving() requires the cs_main lock be held, but the SanityCheckMessage() callback is deep inside the network-servicing code with cs_vRecvMsg lock held. Untangling that is, I think, a project for another day.

... ah, except the java comparison tool doesn't like being disconnected when it sends a big block. That's why Travis is failing.

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

@theuni : new Travis error:
{standard input}: Fatal error: can't close test/test_test_bitcoin-sanity_tests.o: No space left on device

Is there anything I can do to fix that?

@theuni

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

@gavinandresen given that it only happened on the one builder and i've never seen it before, I'd chalk that up to a freak occurrence unless it becomes a trend. Fire it off again?

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2015

It was, indeed, a spurious Travis error.

A note on the Java comparison tool: I believe it will continue to work with this pull request "forever" because it produces down-version blocks for testing; it will never trigger the supermajority-of-miners condition to make bigger blocks legal.

@gavinandresen gavinandresen force-pushed the gavinandresen:blocksize_fork_0.12 branch Jul 1, 2015

@dgenr8

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2015

Been running on mainnet for 1 day. Got a lot of "Oversized addr" disconnects (65% of all connections, in bursts). Don't know what was being received though. Also got one "Oversized headers" disconnect.

gavinandresen added some commits Jul 13, 2015

Refactor: protect mapNodeState with its own lock
Encapsulate mapNodeState in a smart-pointer class with its own lock.

Why? So Misbehaving() can be called from the sanity-check-a-message code without holding cs_main.

And to get better exception safety (the smart-pointer approach gives RAII semantics).

And because protecting fewer things with cs_main is a good idea.

Tested by compiling with -DDEBUG_LOCKORDER, running all of the qa/rpc-tests, and running a node on the main network overnight.
Refactor, new CNode::FinalizeHeader method
I need this to write some unit tests for the
CNode::ReceiveMsgBytes() function.
Implement hard fork to allow bigger blocks
Unit test and code for a bigger-block hard fork.
Parameters are:
  8MB cap
  ... doubling every two years (so 16MB in 2018)
  ... for twenty years
  ... earliest possible chain fork: 11 Jan 2016
  ... after miner supermajority (code in the next patch)
  ... and grace period once miner supermajority achieved (code in next patch)
Implement miner vote and grace period for block size fork
These changes implement and test miner rollout of a bigger
block size hard fork.

qa/rpc-tests/bigblocks.py mines a 50% upgraded -regtest chain,
generates 4MB of transactions, and then tests the upgrade and
mining code to make sure larger blocks are only created when
the fork conditions are met.

The activation state of the fork is stored in the block tree database;
it is written when the threshold is met (and unwritten if the
threshold block is re-orged out of the best chain), and read at startup.

@gavinandresen gavinandresen force-pushed the gavinandresen:blocksize_fork_0.12 branch to b58d925 Jul 14, 2015

@gavinandresen gavinandresen deleted the gavinandresen:blocksize_fork_0.12 branch Jul 15, 2015

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Closing for now; too painful to try to keep up with the movement of code from main.h to policy/policy.h.

@jeffreyrufino

This comment has been minimized.

Copy link

commented Feb 20, 2018

Do you guys think this has affected the Bitcoin Protocol?

@chovy
Copy link

left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.