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

Testchains: Introduce custom chain whose constructor... #8994

Open
wants to merge 8 commits into
base: master
from

Conversation

@jtimon
Copy link
Member

commented Oct 22, 2016

...reads from runtime params and simplify the creation of partitioned chains by simply generating different gensis block hashes from a given custom name.
Datadir now depends on -chain except for the reserved values.

Dependencies:

  • Use a proper factory for creating chainparams #8855
    - [ ] Really don't validate genesis block #9102
  • Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494
  • QA: segwit.py: s/find_unspent/find_spendable_utxo/ #11869
  • Refactor: One CBaseChainParams should be enough #12128

Other features:

  • Uses a custom chain for all python tests.
  • Create new testchains with different genesis hashes at will.
  • Load chainparams from separated file or command line. (file left for later, see https://github.com/jtimon/bitcoin/tree/b16-new-testnet-file )
  • New chains are neither orange, blue nor green: they're purple and have your custom chain petname shown in the GUI.
  • Extra context: some people asked for signed blocks but that's way more disruptive and this is already review-thirsty (see #9177 ).
@btcdrak

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

Concept ACK

@btcdrak

This comment has been minimized.

Copy link
Member

commented Oct 22, 2016

Some ideas from IRC today: Allow users to specify a .json file with all the parameters, and include an option for federated signing with list of signing keys.

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch 3 times, most recently Oct 25, 2016

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch Nov 3, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2016

Needed rebase. Also, new configurable parameter was introduced in #9053

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch 2 times, most recently Nov 3, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2016

Rebased on top of #9102

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch 3 times, most recently Nov 10, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

Since #8855 needed rebase, this one too. Also adapted some more rpc tests to use the custom chain instead of regtest (only 4 missing it seems, but travis should pass).

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch Nov 15, 2016

@jtimon jtimon referenced this pull request Nov 17, 2016

Closed

NOMERGE: WIP: Support block signed custom testchains #9177

0 of 7 tasks complete

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch Nov 18, 2016

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch 3 times, most recently Dec 2, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

Needed rebase.

Update:

  • Now all rpc tests pass for self.chain="regtest"

"Magic" fixes:

  • pruning.py and mempool_packages.py now pass with both regtest and custom (they were passing travis before because they are in the extended set of tests).

"Magic" fails:

  • Now p2p-segwit.py only passes with regtest but not with custom like before

This is related to #9102 (see #9102 (comment) ping @laanwj ) in the sense that we cannot test that the genesis block is not validated unless we can run the system for a chain whose genesis block doesn't comply with the rules.
For example, the custom chain (unlike regtest) doesn't comply with pow (although some values for -chainpetname should make it).

@gmaxwell, you suggested that something like #9177 would need a lot of coverage testing and review, and I completely agree. Any suggestion to advance in that front in this PR instead of #9177 (which currently doesn't work and has one unittest that is not independent) would be welcomed.

You have recently made improvements in the rpc test framework, @MarcoFalke , and are familiar with it. If you get bored and can help fix one of the 3 tests that aren't passing for custom or just comment about the python changes in general, that would be great.

REM:

  • The last 2 commits are not necessary, but anyone feel free to comment on them.
  • Except for p2p-compactblocks.py, segwit.py (and now p2p-segwit.py too) all rpc tests pass with "custom" too
@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

@TheBlueMatt said in #9243 :

I think for things which are their own classes, they should either have the arguments they care about passed into the constructor, or passed in when they need them (eg the way mempool limiting is done now)

The longer version of CreateChainParams could indeed take all fields in CChainParams and Consensus::Params as arguments, but that would break the encapsulation. Every time a field is added or removed all calls would need to be adapted. Note that ideally, in the future, all tests would rely on CreateChainParams but none of them on the chainparams globals (accessed to via SelectParams and Params() ).

Another simpler option would be to simply let chainparams.cpp depend on globals mapArgs and mapMultiArgs (it depends on util.o anyway).

Yet another option is to undo some of that changes in #9243 if that gets merged first, which would be my personal preference.

If anyone has more options, I'm all ears.

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch Dec 3, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2016

Rebased

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch Dec 20, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

Rebased

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch to 3ab01aa Dec 20, 2018

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

Rebased. A new commit undoing recent changes was also needed.
There are still some pending nits/discussions.

@DrahtBot DrahtBot removed the Needs rebase label Dec 20, 2018

@scravy scravy referenced this pull request Feb 24, 2019

Merged

Add -customchainparams #679

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch from 3ab01aa to 093dfe1 Jun 8, 2019

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

Rebased again

@DrahtBot DrahtBot removed the Needs rebase label Jun 8, 2019

@jtimon jtimon referenced this pull request Jun 13, 2019

Open

Bury bip9 deployments #16060

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Added customization for -assumed_blockchain_size and -assumed_chain_state_size introduced in #13216 . I forgot that on rebase.

Also, added basic documentation for all fields as discussed before. Although nits for the documention are of course welcomed, we could perhaps fine tune that and, more importantly, which fields don't even deserve to be exposed, on a different PR, not to add further noise.

In that spirit, I brought back -ndefaultport and -frequirestandard, even though I personally don't think they deserve to be exposed. More generally, I would suggest adding to that list any field containing the word "consider" for its documentation in my initial proposed documentation, sorry for the various redundancies.

They can be discussed individually in different PRs is my main point in bringing them back, I guess. Perhaps we can keep them all as a way to document the existing alternatives people could be using and to remind us that perhaps not all of them even belong in chainparams at all.

Honestly, to me this is mostly about being able to produce many regtest networks with the same binary that are incompatible between them because they don't share the same genesis block hash, which happens to be a consensus rule intrinsically by definition (for any block with height 1 needs to be a child of the genesis block). Perhaps I should add a simple test about that.

To reiterate other motivations, of course it's also for never adding more parameters for each new type of testing chain we want. That scales linearly by the number of chains.
@kallewoof is trying to add some -signet option to make it 3, perhaps 4 in the future, who knows.
I say 3 options it's fine, -chain=signet or even -chain=kalle if he wants should be enough.
If people ever go crazy again and start trading testnet3 for real money, testnet4 could be simply a config file, or even a section starting with:

[testnet4]

Additionally, we will never need a set method for chainparams ever again (I've seen those come and try to come, and go, and I fought for them to go for a while), so our chainparams singleton could be const forever after initialization, just as everybody reasons about it, and as it should IMO.

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch from 093dfe1 to b872b10 Jun 13, 2019

@laanwj laanwj added this to Chasing Concept ACK in High-priority for review Jun 13, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

concept ACK

It makes testing easier, enough said.

instagibbs and others added some commits Nov 7, 2016

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch from b872b10 to 03a735b Jun 14, 2019

jtimon added some commits Oct 21, 2016

Testchains: Introduce custom chain whose constructor...
...reads params from regular arguments

@jtimon jtimon force-pushed the jtimon:0.13-new-testchain branch from 03a735b to 4d8580c Jun 14, 2019

@kallewoof

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Concept ACK and light code re-utACK.

I am not a big fan of adding a ton of new -options but I don't see a way around it without some custom grokkery. The # of lines added (+351-267=+84) seems warranted for the functionality this provides, and I think this would go hand in hand with signet.

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Well, we can leave some of the options undocumented and we can also not expose some of the chain params at all since there are alternative ways to test that functionality (like the port number or allownonstd).
Not sure if that's what you mean by "some custom grokkery though".

@kallewoof

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

I meant a new file format for custom chains that is separate from the main argument stuff. People would either provide a JSON object describing all the options or a file that had the options in it.

That said though, isn't it already possible to do e.g. -chain="{\"name\":\"foochain\", \"optionxyz\": 36, ...}? That would put your command additions at one, but the downside is that error checking would be less obvious (e.g. you would probably have to manually check that no unknown keys are in there).

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Concept ACK. There's two things I'm "Approach NACK" on -- all the extra options, and switching to regtest2.

I think the options can be broken out into:

  • chain params: halving interval, (initial reward?), pow (limit, interval, spacing, retargeting, min chain work)
  • soft-fork params: vbparams, bip16exception, etc
  • p2p params: default port, seednode, pchmessagestart
  • UI elements: bech32 hrp, pubkey,script,secret,extpub,extprv prefixes, assumed chain sizes

Would it make sense to have that stuff be encoded as a hex blob -- that way it can be cut and paste as a unit, and maybe a checksum can be added? You could make that work by running a script to generate it (along with a genesis block, with sufficient proof of work to not need to add exceptions to the tests), and having the script spit out a hex blob. Then you just paste the hex blob into your bitcoind.conf and share elsewhere? So you end up with a few chain specs in your config:

customchain=regtest42:041a0532342fea1...
customchain=kallesig:a41a42340591eaf42...

and can then choose one by saying --chain=kallesig.

I think setting things up that way would make it pretty easy to allow you to have your own newspaper quote, which should in turn make it easy to have regtest use this code path without needing a hardfork.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Not sure about the hex part. That makes it hard to change a param. What about requiring that the specs are provided in a json file (-customchainfile=foo.json) and can not be passed in on the command line?

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

What is the advantage of the blob? How is copying and pasting from conf files harder than running a script to then copy and paste just the same?
If you require proof of work, then the same config for a chain won't necessarily result in the same genesis hash.
You can already "chose your newspaper quote", whatever you put in -chain=hashthis is going to be hashed in the genesis block instead of satoshi's quote.

Regarding moving the tests from regtest to regtest2 (or custom, any name except main, test and regtest would do) is the way to test this. I don't see the disadvantage on moving away from regtest there, they're practically the same and some tests could make use of the custom chainparams.
People can still use the old regtest if they want.

What about requiring that the specs are provided in a json file (-customchainfile=foo.json) and can not be passed in on the command line?

I proposed this in the past, I think I had it implemented at some point. Right, see #8994 (comment)

Now that there are sections for the config file, I don't care so much about it, you can do things like:
https://github.com/jtimon/multi-ln-demo/blob/master/conf/bob_bitcoind.conf
so I don't see what the advantage of using a sepaarated file would be anymore.
But if people prefer that, I can implement that again, or perhaps in a different PR.
Let's see what other people think, I don't even remember the previous discussions about it very clearly myself.

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.