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

BIP100: Dynamic max block size by miner vote #398

Open
wants to merge 3 commits into
base: dev
from

Conversation

Projects
None yet
@dgenr8
Collaborator

dgenr8 commented Mar 24, 2017

This is a Bitcoin Unlimited port of the reference implementation of the BIP100 specification.

As anticipated in BUIP002 Multi-BIP Scaling Enabler, a -bip100 option is added that causes BU to observe rules that coordinate the EB setting among adopting nodes running a BIP100 implementation (Core and XT development versions have been published), as well as using the input of pure EC nodes.

In BIP100 mode, at each difficulty retargeting, miners' block size votes are tallied and EB is automatically moved toward the size that 75% of miners will accept immediately, in 5% change increments. The first such increase will lead to a hard fork, so miners are advised to signal 1MB until ready, just as with pure EC. Subsequent increases or decreases will be followed by all BIP100 nodes without disagreement.

The operation of AD is not altered by -bip100, so a low setting of AD still allows the node to override EB (and exit consensus with other BIP100 nodes).

Full nodes need no configuration to follow the network sizelimit as determined by BIP100. Miners may set their coinbase /B vote using

-maxblocksizevote=<n> Set vote for maximum block size in megabytes (default: bip100 sizelimit)

Several RPC's are enhanced to return the bip100 sizelimit as of the current tip, regardless of whether -bip100 is set.

On first run, block index entries starting at the activation height (449568, which was in January 2017) are updated to track miner size votes and sizelimit history. Use -debug=reindex (NOT -reindex) to see the progress of this update.

When -bip100 is set, a BIP100 flag and the precise current network sizelimit are published in the user agent string (and coinbase for miners) as EB.

Built on #385 which helps the operation of the bip100-sizelimit.py test.

Known TODOs: Update Unlimited GUI as anticipated in BUIP002.

Show outdated Hide outdated src/main.cpp
Show outdated Hide outdated src/main.cpp
Show outdated Hide outdated src/main.cpp
Show outdated Hide outdated src/main.cpp
Show outdated Hide outdated src/main.cpp
// Load block index from databases
if (!fReindex && !LoadBlockIndexDB())
if (!fReindex && !LoadBlockIndexDB(fRebuildRequired))

This comment has been minimized.

@greatwolf

greatwolf Mar 24, 2017

maybe simplify to

return fReindex || LoadBlockIndexDB(fRebuildRequired));
@greatwolf

greatwolf Mar 24, 2017

maybe simplify to

return fReindex || LoadBlockIndexDB(fRebuildRequired));
Show outdated Hide outdated src/maxblocksize.cpp
@deadalnix

This comment has been minimized.

Show comment
Hide comment
@deadalnix

deadalnix Mar 26, 2017

Collaborator

To be frank, i don't think this is a good idea. I'm sure there are ways to game the voting system.

Collaborator

deadalnix commented Mar 26, 2017

To be frank, i don't think this is a good idea. I'm sure there are ways to game the voting system.

@dagurval

This comment has been minimized.

Show comment
Hide comment
@dagurval

dagurval Apr 1, 2017

Collaborator

@dgenr8 Can you cherry-pick this fix for the failing unittests on travis? dagurval/bitcoin@d17dcf6

Collaborator

dagurval commented Apr 1, 2017

@dgenr8 Can you cherry-pick this fix for the failing unittests on travis? dagurval/bitcoin@d17dcf6

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Apr 2, 2017

Collaborator

Pushed fix from @dagurval and BU style updates.

Collaborator

dgenr8 commented Apr 2, 2017

Pushed fix from @dagurval and BU style updates.

krzysztofwos pushed a commit to krzysztofwos/BitcoinUnlimited that referenced this pull request Apr 2, 2017

Merge #398: Test whether ECDH and Schnorr are enabled for JNI
eee808d Test whether ECDH and Schnorr are enabled for JNI (Pieter Wuille)
@sickpig

This comment has been minimized.

Show comment
Hide comment
@sickpig

sickpig Apr 20, 2017

Collaborator

@dgenr8 PR need to be rebased due to conflict in src/main.cpp

Collaborator

sickpig commented Apr 20, 2017

@dgenr8 PR need to be rebased due to conflict in src/main.cpp

@roybadami

This comment has been minimized.

Show comment
Hide comment
@roybadami

roybadami Apr 20, 2017

Contributor

@deadalnix My feeling is that BU is about giving the choice to users of our software. I don't think you have to support BIP100 to support giving uses the option of using it. We're all about developers getting out of the business of telling users (i.e. miners, full nodes) what to do. As far as I'm aware, this approach was agreed in BUIP002.

Contributor

roybadami commented Apr 20, 2017

@deadalnix My feeling is that BU is about giving the choice to users of our software. I don't think you have to support BIP100 to support giving uses the option of using it. We're all about developers getting out of the business of telling users (i.e. miners, full nodes) what to do. As far as I'm aware, this approach was agreed in BUIP002.

@ptschip

This comment has been minimized.

Show comment
Hide comment
@ptschip

ptschip Apr 20, 2017

Collaborator

@dgenr8 This code should be run through clang-format (we use Allman style braces and a few other differences) and should be added to ./src/formatted-files

Collaborator

ptschip commented Apr 20, 2017

@dgenr8 This code should be run through clang-format (we use Allman style braces and a few other differences) and should be added to ./src/formatted-files

Show outdated Hide outdated src/rpc/misc.cpp
@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Apr 20, 2017

Collaborator

@sickpig @ptschip Rebased, formatted new files and added to src/.formatted-files.

Collaborator

dgenr8 commented Apr 20, 2017

@sickpig @ptschip Rebased, formatted new files and added to src/.formatted-files.

@AllanDoensen

This comment has been minimized.

Show comment
Hide comment
@AllanDoensen

AllanDoensen Apr 22, 2017

Contributor

@dgenr8 This is good work. Do you have a reference implementation for BIP100 somewhere?

Also I have been do a bit of QT/GUI work on BU Lately and would welcome the opportunity to build a GUI for BIP100. If you have not started on this I would like to help.

Contributor

AllanDoensen commented Apr 22, 2017

@dgenr8 This is good work. Do you have a reference implementation for BIP100 somewhere?

Also I have been do a bit of QT/GUI work on BU Lately and would welcome the opportunity to build a GUI for BIP100. If you have not started on this I would like to help.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Apr 22, 2017

Collaborator

@AllanDoensen That would be great. We've done no GUI work for this for any platform. For reference see https://bip100.tech

Collaborator

dgenr8 commented Apr 22, 2017

@AllanDoensen That would be great. We've done no GUI work for this for any platform. For reference see https://bip100.tech

@gandrewstone

This comment has been minimized.

Show comment
Hide comment
@gandrewstone

gandrewstone Apr 27, 2017

Collaborator

there are a lot of comments but no signoff. Who has pulled this PR and tested thoroughly? "Drive by" reviews are of very limited usefulness. In fact they sometimes give the wrong impression of a carefully discussed and tested feature.

Collaborator

gandrewstone commented Apr 27, 2017

there are a lot of comments but no signoff. Who has pulled this PR and tested thoroughly? "Drive by" reviews are of very limited usefulness. In fact they sometimes give the wrong impression of a carefully discussed and tested feature.

@ftrader

This comment has been minimized.

Show comment
Hide comment
@ftrader

ftrader Apr 27, 2017

Collaborator

I think there is an expectation from contributing reviewers that code review is one activity, and thorough testing is another.

And to a large extent, I agree with this view and think that they are two separate activities, and the testing activity needs to be done as @gandrewstone points out, but I think we need to appreciate all review contributions, and arrange testing formally to ensure it's done.

That applies to all feature PRs.

I certainly don't know what the in place "signoff" procedures are, and maybe that's the case for others here too. In that case we should document how we want this to work. Apologies if I missed some existing documentation that explains the procedure - feel free to point me to it.

My suggestion would be to coordinate on the required level of verification as soon as a PR is submitted, and make it explicit here in Github, e.g. through "Assigned" field and labels.

Collaborator

ftrader commented Apr 27, 2017

I think there is an expectation from contributing reviewers that code review is one activity, and thorough testing is another.

And to a large extent, I agree with this view and think that they are two separate activities, and the testing activity needs to be done as @gandrewstone points out, but I think we need to appreciate all review contributions, and arrange testing formally to ensure it's done.

That applies to all feature PRs.

I certainly don't know what the in place "signoff" procedures are, and maybe that's the case for others here too. In that case we should document how we want this to work. Apologies if I missed some existing documentation that explains the procedure - feel free to point me to it.

My suggestion would be to coordinate on the required level of verification as soon as a PR is submitted, and make it explicit here in Github, e.g. through "Assigned" field and labels.

counts = [ x.getblockcount() for x in rpc_connections ]
if counts == [ counts[0] ]*len(counts):
return True
return False

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

why not use util.sync_blocks()?

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

why not use util.sync_blocks()?

This comment has been minimized.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

The version in util.py waits forever. We rely on sync failure to detect test failures.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

The version in util.py waits forever. We rely on sync failure to detect test failures.

This comment has been minimized.

@gandrewstone

gandrewstone May 1, 2017

Collaborator

it seems like adding an optional timeout parameter to the version in util would be valuable

@gandrewstone

gandrewstone May 1, 2017

Collaborator

it seems like adding an optional timeout parameter to the version in util would be valuable

This comment has been minimized.

@dgenr8

dgenr8 May 1, 2017

Collaborator

If it's ok, I will do that in a separate PR that follows.

@dgenr8

dgenr8 May 1, 2017

Collaborator

If it's ok, I will do that in a separate PR that follows.

@@ -150,6 +153,15 @@ class CBlockIndex
//! (memory only) Sequential id assigned to distinguish order in which blocks are received.
uint32_t nSequenceId;
//! Index entry serial format version

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

the comment isn't helping me much. How is this different from nVersion?

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

the comment isn't helping me much. How is this different from nVersion?

This comment has been minimized.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

We need to track nSerialVersion in the CBlockIndex object. The existing nVersion in the object is the actual block header version. nSerialVersion is read/written to the old nVersion location at the beginning of the serialized CBlockIndex object.

The ubiquitous serialization nType/nVersion parameters have been removed in a later core release.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

We need to track nSerialVersion in the CBlockIndex object. The existing nVersion in the object is the actual block header version. nSerialVersion is read/written to the old nVersion location at the beginning of the serialized CBlockIndex object.

The ubiquitous serialization nType/nVersion parameters have been removed in a later core release.

Show outdated Hide outdated src/rpc/blockchain.cpp
return 0;
return static_cast<uint64_t>(FindVote(s)) * 1000000;

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

fractional EBs are allowed. For example "/EB3.5/". This line and FindVote need to be changed to handle fractions. Please see https://github.com/BitcoinUnlimited/BUIP/blob/master/005.mediawiki for the exact format of the EB field...

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

fractional EBs are allowed. For example "/EB3.5/". This line and FindVote need to be changed to handle fractions. Please see https://github.com/BitcoinUnlimited/BUIP/blob/master/005.mediawiki for the exact format of the EB field...

This comment has been minimized.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

We specified only integral EB megabyte signals would count as votes, although in -bip100 mode, BU will publish (and enforce) EB with up to 6 decimal places (the need for which arises from the 5% change cap).

The reasons were to keep the semantics of B and EB the same, and to avoid decimal parsing logic. The bip looks forward to a time when there will be several significant digits to the left of the decimal point.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

We specified only integral EB megabyte signals would count as votes, although in -bip100 mode, BU will publish (and enforce) EB with up to 6 decimal places (the need for which arises from the 5% change cap).

The reasons were to keep the semantics of B and EB the same, and to avoid decimal parsing logic. The bip looks forward to a time when there will be several significant digits to the left of the decimal point.

This comment has been minimized.

@gandrewstone

gandrewstone May 1, 2017

Collaborator

Ok, in thinking about it more I am not that happy with rounding /EB1.9/ down because the block size at a 1.05 maximum increase is definitely going to be ticking up in chunks that are less than 1MB. So the miner needs to understand that 2 precisions are being used for the same value. But ok, I see how it simplifies the code.

But I think that interpreting /EB3.5/ as no vote (and therefore an implicit vote for the current limit) is a big mistake. Large block votes may be highly underrepresented. I think we need to round this down to the nearest whole number (i.e. 3). This change is not that bad -- it remains limited to FindVote and since we are rounding down perfect representation does not matter. So we can lexical cast to a float and then round down to a uint64_t.

@gandrewstone

gandrewstone May 1, 2017

Collaborator

Ok, in thinking about it more I am not that happy with rounding /EB1.9/ down because the block size at a 1.05 maximum increase is definitely going to be ticking up in chunks that are less than 1MB. So the miner needs to understand that 2 precisions are being used for the same value. But ok, I see how it simplifies the code.

But I think that interpreting /EB3.5/ as no vote (and therefore an implicit vote for the current limit) is a big mistake. Large block votes may be highly underrepresented. I think we need to round this down to the nearest whole number (i.e. 3). This change is not that bad -- it remains limited to FindVote and since we are rounding down perfect representation does not matter. So we can lexical cast to a float and then round down to a uint64_t.

This comment has been minimized.

@dgenr8

dgenr8 May 1, 2017

Collaborator

@gandrewstone I couldn't find any EB tags with a non-zero decimal. Do you think it will be hard to get Slush to change 16.0 to 16?

@dgenr8

dgenr8 May 1, 2017

Collaborator

@gandrewstone I couldn't find any EB tags with a non-zero decimal. Do you think it will be hard to get Slush to change 16.0 to 16?

Show outdated Hide outdated src/maxblocksize.cpp
uint64_t GetNextMaxBlockSize(const CBlockIndex *pindexLast, const Consensus::Params &params)
{
// BIP100 not active, return legacy max size
if (pindexLast == NULL || pindexLast->nHeight < params.bip100ActivationHeight)

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

what will happen if params.bip100ActivationHeight is not a multiple of params.DifficultyAdjustmentInterval()?

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

what will happen if params.bip100ActivationHeight is not a multiple of params.DifficultyAdjustmentInterval()?

This comment has been minimized.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

The effect on next max block size would be the same as if bip100ActivationHeight were set to the first block of the next difficulty interval.

@dgenr8

dgenr8 Apr 28, 2017

Collaborator

The effect on next max block size would be the same as if bip100ActivationHeight were set to the first block of the next difficulty interval.

{
try
{
return boost::lexical_cast<uint32_t>(std::string(begin(curr) + 1, end(curr)));

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

this code accepts a vote of 0, although that is illegal as per BIP100

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

this code accepts a vote of 0, although that is illegal as per BIP100

This comment has been minimized.

@dagurval

dagurval Apr 28, 2017

Collaborator

Good find. It should continue to look for votes in the coinbase in case of 0.

@dagurval

dagurval Apr 28, 2017

Collaborator

Good find. It should continue to look for votes in the coinbase in case of 0.

This comment has been minimized.

@dgenr8

dgenr8 Apr 29, 2017

Collaborator

Indeed a good find. Both the specification and the implementation may need to be updated and any changes will need to be made on the other platforms as well. Thanks @gandrewstone.

@dgenr8

dgenr8 Apr 29, 2017

Collaborator

Indeed a good find. Both the specification and the implementation may need to be updated and any changes will need to be made on the other platforms as well. Thanks @gandrewstone.

This comment has been minimized.

@dgenr8

dgenr8 May 1, 2017

Collaborator

Changes pushed to implement this specification update. EB0 (if anyone does such a thing) now behaves the same as B0 - it is an explicit vote for the current limit.

@dgenr8

dgenr8 May 1, 2017

Collaborator

Changes pushed to implement this specification update. EB0 (if anyone does such a thing) now behaves the same as B0 - it is an explicit vote for the current limit.

Show outdated Hide outdated src/maxblocksize.cpp
uint64_t lowerValue = votes.at(params.nMaxBlockSizeChangePosition - 1);
uint64_t raiseValue = votes.at(params.DifficultyAdjustmentInterval() - params.nMaxBlockSizeChangePosition);
assert(lowerValue >= 1000000); // minimal vote supported is 1MB

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

this is a bit dangerous, since the FindVote accepts 0. Regardless of whether the FindVote gets fixed, I'd prefer a forcing function here not an assert, because BIP100 makes is clear that 1MB is the lowest you can go and so if somehow other portions of the code get "faked out" we can fall back on that value rather than crashing:
if (lowerValue < 1000000) lowerValue = 1000000;

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

this is a bit dangerous, since the FindVote accepts 0. Regardless of whether the FindVote gets fixed, I'd prefer a forcing function here not an assert, because BIP100 makes is clear that 1MB is the lowest you can go and so if somehow other portions of the code get "faked out" we can fall back on that value rather than crashing:
if (lowerValue < 1000000) lowerValue = 1000000;

This comment has been minimized.

@dagurval

dagurval Apr 28, 2017

Collaborator

FindVote returning 0 means "no vote, use current limit". See the for loop above where we check for 0. If lower value is less than 1MB then it's a bug. Since this is consensus critical we should assert on the bug.

@dagurval

dagurval Apr 28, 2017

Collaborator

FindVote returning 0 means "no vote, use current limit". See the for loop above where we check for 0. If lower value is less than 1MB then it's a bug. Since this is consensus critical we should assert on the bug.

return nMaxBlockSize;
}
static uint32_t FindVote(const std::string &coinbase)

This comment has been minimized.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

is a vote uint32_t or uint64_t (as specified in GetNextMaxBlockSize)? Let's be consistent even if it doesn't matter.

@gandrewstone

gandrewstone Apr 28, 2017

Collaborator

is a vote uint32_t or uint64_t (as specified in GetNextMaxBlockSize)? Let's be consistent even if it doesn't matter.

This comment has been minimized.

@dagurval

dagurval Apr 28, 2017

Collaborator

This helper function (not exported) returns the vote as MB (as defined in coinbase). It must be safe to multiply it with 1000000 when converting to bytes. Byte representation is always uint64_t.

@dagurval

dagurval Apr 28, 2017

Collaborator

This helper function (not exported) returns the vote as MB (as defined in coinbase). It must be safe to multiply it with 1000000 when converting to bytes. Byte representation is always uint64_t.

This comment has been minimized.

@gandrewstone

gandrewstone May 1, 2017

Collaborator

but this is returning uint32_t so byte representation is not "always uint64_t"...

@gandrewstone

gandrewstone May 1, 2017

Collaborator

but this is returning uint32_t so byte representation is not "always uint64_t"...

This comment has been minimized.

@dgenr8

dgenr8 May 1, 2017

Collaborator

The uint32_t represents megabytes. This implementation effectively limits the network max block size to 4.3 petabytes.

We accept the risk that old votes for values larger than this may need to be special-cased to avoid re-interpretation if it becomes necessary to support blocks larger than this.

@dgenr8

dgenr8 May 1, 2017

Collaborator

The uint32_t represents megabytes. This implementation effectively limits the network max block size to 4.3 petabytes.

We accept the risk that old votes for values larger than this may need to be special-cased to avoid re-interpretation if it becomes necessary to support blocks larger than this.

@sickpig sickpig added the consensus label Apr 28, 2017

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 May 24, 2017

Collaborator

Rebased.

Collaborator

dgenr8 commented May 24, 2017

Rebased.

dgenr8 added some commits Jan 7, 2017

BIP100: Dynamic max block size by miner vote
With Dagur Johannsson <dagurval@pvv.ntnu.no>
BIP100: Unit and regression tests
Unit tests for GetNextMaxBlockSize and FindVote.
Multi-node voting regression test.

With With Dagur Johannsson <dagurval@pvv.ntnu.no>
Includes regtest routines by Gavin Andresen <gavinandresen@gmail.com>

@sickpig sickpig reopened this Jan 9, 2018

@dgenr8 dgenr8 closed this Sep 19, 2018

@dgenr8 dgenr8 deleted the dgenr8:bu-bip100 branch Sep 19, 2018

@dgenr8 dgenr8 restored the dgenr8:bu-bip100 branch Sep 19, 2018

@dgenr8 dgenr8 reopened this Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment