Paths

Table of Contentst

Added a script to generate chainparams constants header files
AbandonedPublic
Actions

Authored by jasonbcox on Jul 1 2019, 17:39.

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Summary

This makes updating chainparams constants trivial.

In the near future, this pipeline becomes possible (all TODO):

A bot with up-to-date nodes runs this script.
Smoke tests are run on the result and landed on master automatically.

For now, this script can be used manually.

Completes T663

Test Plan

make check
ninja check
ninja check-devtools

show help message
generate_chainparams_constants.py --help

errors due to missing config/config params
generate_chainparams_constants.py -c blah
generate_chainparams_constants.py
vim ~/.bitcoin/bitcoin.conf # add rpcuser
generate_chainparams_constants.py
vim ~/.bitcoin/bitcoin.conf # add rpcpassword

run bitcoind for both mainnet and testnet
bitcoind
bitcoind --testnet

generates mainnet output
generate_chainparams_constants.py

generates testnet output
generate_chainparams_constants.py -a "127.0.0.1:18332"

saves aggregated output to file
generate_chainparams_constants.py -o ../../../src/chainparamsconstants.h
generate_chainparams_constants.py -a "127.0.0.1:18332" -o ../../../src/chainparamsconstants.h

saves aggregated output to file (generates the header you find in this diff)
generate_chainparams_constants.py -o ../../../src/chainparamsconstants.h -b 000000000000000004425aeeb553758650da20031643fbfff0f53ac5dd1b39c3
generate_chainparams_constants.py -a "127.0.0.1:18332" -o ../../../src/chainparamsconstants.h -b 000000000000037b3419370d2306ada6b4f30ac9f01a1bec513eef69b8cd4eab

Diff Detail

Repository

rABC Bitcoin ABC

Branch

genbestchainparams

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6803
Build 11653: Bitcoin ABC Buildbot (legacy)
Build 11652: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jul 1 2019, 17:39

Herald added a reviewer: deadalnix. · View Herald TranscriptJul 1 2019, 17:39

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6597: Diff 9864.Jul 1 2019, 17:46

Fabien requested changes to this revision.Jul 1 2019, 20:31

Fabien added a subscriber: Fabien.

Fabien added inline comments.

contrib/devtools/generate-bestchainparams.sh
9 ↗	(On Diff #9864)	The script help message doesn't match the code, it states that every argument is an option while 2 over 3 are mandatory.
54 ↗	(On Diff #9864)	`[0-9a-f]` is enough
65 ↗	(On Diff #9864)	Dito

This revision now requires changes to proceed.Jul 1 2019, 20:31

deadalnix requested changes to this revision.Jul 1 2019, 20:41

deadalnix added inline comments.

contrib/devtools/generate-bestchainparams.sh
90 ↗	(On Diff #9864)	So this is just formatting a string? You realize that the caller will have to format a string, with the parameters, to create a command line? This part of the patch is absolutely not useful.
src/Makefile.am
121 ↗	(On Diff #9864)	Why do you need two files ?
src/chainparamsbestchainmainnet.h
5 ↗	(On Diff #9864)	Use the same format as for other generated files so they actually are recognized as generated (and also because there is no point reinventing the wheel every time you need a wheel).
15 ↗	(On Diff #9864)	Why aren't these constants? Also, what is inline supposed to achieve here?

Overhaul of the script based on feedback:

Script calls bitcoin-cli to get necessary information. This is a departure from previous attempts at using the HTTP RPC because HTTP RPC has only a limited set of RPC commands available, as well as removing the security question of distributing the node calls across a network by default.
Script generates output that doesn't depend on source control or any part of the current code state.
Output is a single header of constants.
Script rewritten in python to be more easily testable.
Better user experience by providing sane default behavior while still being easy to override.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Apparently @generated applies anywhere in the file. Fixed by splitting the string in generate_bestchainparams.py so that the file is not recognized as generated.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6803: Diff 10205.Jul 10 2019, 18:18

jasonbcox edited the test plan for this revision. (Show Details)Jul 10 2019, 18:19

jasonbcox edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6805: Diff 10209.Jul 10 2019, 18:23

jasonbcox mentioned this in D3193: Added update-chainparams.py to fetch and replace assume valid block values from a node using HTTP RPC.Jul 10 2019, 18:25

jasonbcox edited the summary of this revision. (Show Details)

This is looking more like something, but:
1/ Naming is terrible, not even descriptive of anything.
2/ It seems strange to me that you need to rely on bitcoin-cli, when both bitcoin-cli and the test framework - this last one being in python, there may even be code that is reusable - manage to fetch the infos directly. Calling into the shell brings a ton fo baggage, especially since, presumably, this will be called from a shell to begin with.
3/ This require a fulling synced node for mainnet and testnet on the same machine running the script.
4/ It doesn't look like there is a way to only update mainnet or testnet.

contrib/devtools/generate_bestchainparams.py
20 ↗	(On Diff #10209)	This is how you create a shell injection attack.
63 ↗	(On Diff #10209)	That seems to be the actual parameter you'd want to configure from the command line rather than the block itself. Maybe the block itself is useful as a parameter, but from an automation standpoint, this is where it is at. For instance, 2 may be enough when you do it manually, and, by the time you put the release out you can check no reorg occured, but if the goal is to automate, is 2 really enough of a margin?
75 ↗	(On Diff #10209)	Reusing the chainwork from anything but the tip makes no sense whatsoever. If the tip end up being reorged, it'll be by a block that has more work anyway so the value isn't invalidated.
src/chainparamsbestchain.h
3 ↗	(On Diff #10209)	Nobody wrote that file, this is irrelevant blurb.
14 ↗	(On Diff #10209)	ChainParambestChain doesn't mean anything.
18 ↗	(On Diff #10209)	constant are capitalized snake case.

This revision now requires changes to proceed.Jul 11 2019, 00:45

All of my attempts to wrap these under a single name have failed (with the intent to discuss these values as a single idea). I'll use ChainParamsConstants for now, but I'll need input if you think the name can be better.

2/3. I'm working on it now, and it's interesting that multiple problems are solved by reusing the test framework code:

- Code is more concise.
- Less code to maintain.
- No shell injection vulnerability.
- As originally intended (but not achieved by the previous iteration), the script can communicate easily with remote nodes without crafting a special bitcoin.conf or bitcoin-cli string for the purpose.
This was a slightly more challenging problem. I was wrestling with this idea before, but was focused on using i/o redirection. Giving the same file as stdin and stdout results in the file being erased before reading. I finally came up with a simple way of reading input and outputting to the same file without complex code, utilizing script arguments instead.

contrib/devtools/generate_bestchainparams.py
63 ↗	(On Diff #10209)	My thought process on this was: If you're running the script manually and want to choose a block, just pick the block. Otherwise, running with defaults or as an automated process has no reason to change how many blocks from the tip is selected. The conclusion is that configuring this '# of blocks from the tip' value is not useful.
75 ↗	(On Diff #10209)	Good point. There's no reason for the assumevalid and chainwork to be tightly coupled.
src/chainparamsbestchain.h
3 ↗	(On Diff #10209)	I wasn't aware that the copyright notice was only for human generated content. I guess it doesn't really matter.

Naming changed to generate_chainparams_constants, etc.
Utilize existing python code where appropriate.
Support for partial updates.
Other changes according to feedback.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox retitled this revision from Added a script to generate bestchainparams header files to Added a script to generate chainparams constants header files.Jul 15 2019, 21:08

jasonbcox edited the summary of this revision. (Show Details)

jasonbcox edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B6862: Diff 10302.Jul 15 2019, 21:13

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

This is both more complex and less useful than how the seeds are built. What is the problem with the way things are done for seeds? If there is one, what is it, and if there is none, why do it differently?

This revision now requires changes to proceed.Jul 22 2019, 19:16

jasonbcox mentioned this in D3732: Added a script to generate chainparams intermediate files.Jul 24 2019, 21:49

Splitting this off into other diffs since a two-part design facilitates compartmentalized review anyway. Part 1 is D3732

jasonbcox mentioned this in rABC3542fbfd24a0: Added a script to generate chainparams intermediate files.Sep 6 2019, 00:31

jasonbcox mentioned this in rSTAGING3542fbfd24a0: Added a script to generate chainparams intermediate files.Sep 19 2019, 02:09

Part 2: D4120