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

DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes #6382

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jtimon
Member

jtimon commented Jul 6, 2015

This creates std::numeric_limits<uint64_t>::max() new testchains, each one with a different maximum block size and genesis block.
It would be generally good to have more people collecting data and
conduction simulations related to different consensus maximum block sizes.
This PR attempts to simplify that work.
Even if it may take long until it is merged (because it requires many
little steps to be taken first), this branch (or a fork of it) can be used right now for
testing purposes.

One can use it, for example, like this: ./src/qt/bitcoin-qt -chain=sizetest -debug -printtoconsole -gen=1 -genproclimit=20 -blocksize=2000000

I will rebase and update the list of dependencies accordingly as
things get merged.

Dependencies:

  • CTestNetParams and CRegTestParams extend directly from CChainParams #6381
  • Chainparams: Translations: DRY: options and error strings #6235
  • Qt: BIP70: Chainparams: DRY: Make qt/guiutil.cpp fit BIP70 chain name strings #6908 (trivial review)
  • Chainparams: Use a regular factory for creating chainparams #6907 (medium review)
  • Don't check the genesis block #6597 (medium review)
  • Blocksize: Turn MAX_BLOCK_SIZE and MAX_BLOCK_SIGOPS into functions #6625
  • Generic selection with -chain= in addition of -testnet and -regtest [closed] #5229
  • Qt: Simplify network/chain styles and add a default purple one (trivial review)
  • Checkpoints: The hash of the genesis block is the genesis checkpoint and chain id [closed] #6230
  • Chainparams: Adapt qt/paymentserver to support more than 2 chains
  • Chainparams: Introduce AssignBase58PrefixStyle() static function NACK
  • Blocksize: Introduce sizetest mode with infinite chains NACK

Similar branches on top of older versions:

https://github.com/jtimon/bitcoin/tree/chainparams-sizetest-0.11

@jtimon jtimon changed the title from DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes to WIP: Chainparams: Introduce N testnet chains to test different block sizes Jul 7, 2015

@laanwj laanwj added the Tests label Jul 17, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 21, 2015

Member

Rebased after #6381. Also rebased back in time to 0.11: https://github.com/jtimon/bitcoin/tree/chainparams-sizetest-0.11

Member

jtimon commented Jul 21, 2015

Rebased after #6381. Also rebased back in time to 0.11: https://github.com/jtimon/bitcoin/tree/chainparams-sizetest-0.11

@JornC

This comment has been minimized.

Show comment
Hide comment
@JornC

JornC Jul 30, 2015

Spun up a node and block explorer on top of this PR:

http://sizenet.yogh.io/#json:getinfo
http://sizenet.yogh.io/#json:getblockchaininfo

Availability subject to change, will jump back to the main chain when no longer needed - if needed at all.

(still synching)

JornC commented Jul 30, 2015

Spun up a node and block explorer on top of this PR:

http://sizenet.yogh.io/#json:getinfo
http://sizenet.yogh.io/#json:getblockchaininfo

Availability subject to change, will jump back to the main chain when no longer needed - if needed at all.

(still synching)

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Aug 21, 2015

Member

Rebased and fixed (see #6077 (comment) ).

Member

jtimon commented Aug 21, 2015

Rebased and fixed (see #6077 (comment) ).

@jtimon jtimon changed the title from WIP: Chainparams: Introduce N testnet chains to test different block sizes to DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes Aug 27, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 4, 2015

Member

This wasn't passing travis because of a small bug in jtimon@989e374 (#6625). I will wait for travis to approve this to force push #6625.

Member

jtimon commented Sep 4, 2015

This wasn't passing travis because of a small bug in jtimon@989e374 (#6625). I will wait for travis to approve this to force push #6625.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 2, 2015

Contributor

NACK. I think the code should exist for people to use for testing, but I dont think Bitcoin Core is the place to support a ton of test/benchmark chains. This is different from the regtest/testnet chains, which exist to create a bitcoin-in-a-box for testing, but which try to mimic Bitcoin where convinient for testing, not to test changes to Bitcoin itself.

Contributor

TheBlueMatt commented Oct 2, 2015

NACK. I think the code should exist for people to use for testing, but I dont think Bitcoin Core is the place to support a ton of test/benchmark chains. This is different from the regtest/testnet chains, which exist to create a bitcoin-in-a-box for testing, but which try to mimic Bitcoin where convinient for testing, not to test changes to Bitcoin itself.

@priestc

This comment has been minimized.

Show comment
Hide comment
@priestc

priestc Oct 20, 2015

I think it might be a better idea to just make one testnet with unlimited blocksize, rather than multiple networks with all different size limits. What is the 20MB test chain going to teach us that the 50MB chain will? When testing scalability, it makes the most sense to have as many people working together than to have multiple people working on multiple chains. Take a look at the bip that I recently wrote that deals with the same subject: bitcoin/bips#222

priestc commented Oct 20, 2015

I think it might be a better idea to just make one testnet with unlimited blocksize, rather than multiple networks with all different size limits. What is the 20MB test chain going to teach us that the 50MB chain will? When testing scalability, it makes the most sense to have as many people working together than to have multiple people working on multiple chains. Take a look at the bip that I recently wrote that deals with the same subject: bitcoin/bips#222

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 20, 2015

Member

I think it might be a better idea to just make one testnet with unlimited blocksize.

You can effectively do that as well (just set -blocksize to std::numeric_limits<uint64_t>::max()). But this also allows you to simulate networks with smaller sizes and to test schism forks scenarios.

@TheBlueMatt what about the individual steps (to make things like this more maintainable on top of master)? What about #6235 and #6597, for example?

Member

jtimon commented Oct 20, 2015

I think it might be a better idea to just make one testnet with unlimited blocksize.

You can effectively do that as well (just set -blocksize to std::numeric_limits<uint64_t>::max()). But this also allows you to simulate networks with smaller sizes and to test schism forks scenarios.

@TheBlueMatt what about the individual steps (to make things like this more maintainable on top of master)? What about #6235 and #6597, for example?

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 21, 2015

Member

Rebased

Member

jtimon commented Oct 21, 2015

Rebased

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 27, 2015

Member

NACK. I think the code should exist for people to use for testing, but I dont think Bitcoin Core is the place to support a ton of test/benchmark chain

I tend to agree with @bluematt here. Let's not overcomplicate bitcoind with all kinds of specific purpose test code.

Member

laanwj commented Oct 27, 2015

NACK. I think the code should exist for people to use for testing, but I dont think Bitcoin Core is the place to support a ton of test/benchmark chain

I tend to agree with @bluematt here. Let's not overcomplicate bitcoind with all kinds of specific purpose test code.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 29, 2015

Member

Precisely this PR makes creating a new testchain easier by removing specific purpose code for different chains that is not always properly encapsulated. Then it prepares code for a future change in block size and then it adds a new testchain to test different block sizes. The last part only makes sense if it is a single extremely simple commit like jtimon@b7bce5c . But even if that change is never accepted, I will maintain it here because that's the way I test "is it easy to create a new testchain?" every time I rebase. I have marked that step as nacked and solved in the first post. I have also properly marked the PRs that are currently closed and opened #6907 and #6908 which are now much more clear after #6235.
People interested in changes to make it easier to create new testchains can follow this PR and I can rebase just one long branch plus only a few little opened PRs at a time.
When there's only nacked commits remaining we can revisit the nacks and merge or close. This was never intended to be merged all at once, that's why it has the bullet points (which I'm finding very useful).
In the meantime, it is more useful to look at this just to "see the big picture", while we focus review on the few open PRs this depends on.

Member

jtimon commented Oct 29, 2015

Precisely this PR makes creating a new testchain easier by removing specific purpose code for different chains that is not always properly encapsulated. Then it prepares code for a future change in block size and then it adds a new testchain to test different block sizes. The last part only makes sense if it is a single extremely simple commit like jtimon@b7bce5c . But even if that change is never accepted, I will maintain it here because that's the way I test "is it easy to create a new testchain?" every time I rebase. I have marked that step as nacked and solved in the first post. I have also properly marked the PRs that are currently closed and opened #6907 and #6908 which are now much more clear after #6235.
People interested in changes to make it easier to create new testchains can follow this PR and I can rebase just one long branch plus only a few little opened PRs at a time.
When there's only nacked commits remaining we can revisit the nacks and merge or close. This was never intended to be merged all at once, that's why it has the bullet points (which I'm finding very useful).
In the meantime, it is more useful to look at this just to "see the big picture", while we focus review on the few open PRs this depends on.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 10, 2015

Member

Rebased after #6908 has being merged.

Member

jtimon commented Nov 10, 2015

Rebased after #6908 has being merged.

@jtimon jtimon referenced this pull request Nov 18, 2015

Closed

DEPENDENT: Globals: Avoid calling Params() #5970

9 of 14 tasks complete

jtimon and others added some commits May 22, 2015

consensus: teach CheckTransaction to check for an arbitrary max tx size
This is a no-op change.

Tests use a value of std::numeric_limits<uint64_t>::max() where necessary, to ensure that they're never
rejected when size isn't being tested.
consensus: Move consensus constants into Consensus::Params and consen…
…sus.h (as functions)

The following are now tied to a chain rather than being defined as global
constants. Their values have not changed.

nMinTxSize
nMaxBlockSize
nMaxTxSize
nMaxBlockSigops
nCoinbaseMaturity

Also, for free (diff-wise):

Blocksize: Turn MAX_BLOCK_SIZE (nMaxBlockSize) and MAX_BLOCK_SIGOPS (nMaxBlockSigops) into functions

...which take Consensus::Params as parameter
This will be convenient to reduce the diff of any proposal that changes the blocksize as a hardfork
consensus: teach ExtractMatches to check for an arbitrary max transac…
…tion number

This is a no-op change. For now, everything passes MAX_BLOCK_SIZE / 60, so the
result matches what it would've before.

Tests use a number equal to the number of transactions where necessary,
to ensure that they're never rejected when blocksizesize isn't being tested.
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 13, 2016

Member

Closing. It will be eventually replaced with a similar one that doesn't use block size as an example.

Member

jtimon commented Jan 13, 2016

Closing. It will be eventually replaced with a similar one that doesn't use block size as an example.

@jtimon jtimon closed this Jan 13, 2016

jtimon referenced this pull request in sipa/bitcoin Apr 6, 2016

Testchains: Don't check the genesis block
Rebased by Pieter Wuille.
Cleanup by Matt Corallo.

jtimon referenced this pull request in sipa/bitcoin Apr 6, 2016

@jtimon jtimon referenced this pull request Oct 1, 2016

Merged

Use a proper factory for creating chainparams #8855

6 of 6 tasks complete

jtimon added a commit to jtimon/bitcoin that referenced this pull request Mar 24, 2017

8855: Use a proper factory for creating chainparams
- Chainparams: Use a regular factory for creating chainparams
- Chainparams: Get rid of CChainParams& Params(std::string)
- Chainparams: Use the factory for pow tests

Upstream: [0.13-chainparams-factory] (replaces #6382)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment