Page MenuHomePhabricator

Added update-chainparams.py to fetch and replace assume valid block values from a node using HTTP RPC
Needs ReviewPublic

Authored by jasonbcox on Tue, Jun 4, 00:46.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

First step towards automating releases (T663). This eliminates manually copy-pasting the blockhash + chainwork.

Prerequisite: You must have bitcoind running with --rest=1 set either as a CLI arg or in the config.
In the near future, we will have a dedicated node that can have these requests directed to (at that point,
the hostaddr default will change from localhost to that dedicated node).

Typical usage during a release is to replace mainnet params (update-chainparams.py with no args set).
Occassionally, testnet params need to be updated as well, and may be done with update-chainparams.py -t.
Both of these uses assume default node settings regarding rpcport, rpcusername, rpcpassword, etc.

Test Plan
# Shows help message
update-chainparams.py -h

# Test that the script can run from locations other than contrib/devtools
# from toplevel
contrib/devtools/update-chainparams.py

# Mainnet Tests
# ----------------
bitcoind --rest=1

# Default works as expected (mainnet chainparams replaced with best tip)
update-chainparams.py

# Trying testnet without the correct hostport gives an error
update-chainparams.py -t
update-chainparams.py -t -p 18332

# Specifying a blockhash replaces chainparams with that block and it's chainwork instead
update-chainparams.py -b <someblockhash>

# Testnet Tests
# ---------------
bitcoind --rest=1 --testnet

# Default throws an error because it expects mainnet
update-chainparams.py

# Testnet works as expected (testnet chainparams replaced with best tip)    
update-chainparams.py -t

# Specifying a blockhash replaces testnet chainparams with that block and it's chainwork instead
update-chainparams.py -t -b <someblockhash>

# Specifying an invalid blockhash errors as expected
update-chainparams.py -t -b 123

# Specifying a valid blockhash that doesn't exist errors as expected
update-chainparams.py -t -b <a valid but non-existent block hash>
update-chainparams-test.py
ninja check-devtools
ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
updatechainparams
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6408
Build 10863: Bitcoin ABC Teamcity Staging
Build 10862: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox edited the summary of this revision. (Show Details)Tue, Jun 4, 00:49
deadalnix requested changes to this revision.Tue, Jun 4, 00:49

The elephant in the room is that this doesn't set checkpoints.

This revision now requires changes to proceed.Tue, Jun 4, 00:49
jasonbcox updated this revision to Diff 9119.Tue, Jun 4, 16:09

Rename to something more sane

jasonbcox retitled this revision from Added set-checkpoint.py to fetch and replace checkpoint values from a node using HTTP RPC to Added update-assumevalid.py to fetch and replace assume valid block values from a node using HTTP RPC.Tue, Jun 4, 16:10
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)Tue, Jun 4, 16:13
jasonbcox edited the summary of this revision. (Show Details)Tue, Jun 4, 16:32
jasonbcox updated this revision to Diff 9124.Tue, Jun 4, 19:22

Rename to update-chainparams.py

jasonbcox retitled this revision from Added update-assumevalid.py to fetch and replace assume valid block values from a node using HTTP RPC to Added update-chainparams.py to fetch and replace assume valid block values from a node using HTTP RPC.Tue, Jun 4, 19:23
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the summary of this revision. (Show Details)Tue, Jun 4, 19:23
deadalnix requested changes to this revision.Wed, Jun 5, 01:30

If you abstract the node in some class and call from there, you can mock it and unit test the whole thing without a node.

contrib/devtools/update-chainparams.py
11 ↗(On Diff #9124)

bitcoin-cli doesn't need this. How come ?

Also, multiline stings are great.

44 ↗(On Diff #9124)

This doesn't need to be in its own function. It just cause the flow to jump back and forth for no good reasons.

This revision now requires changes to proceed.Wed, Jun 5, 01:30
jasonbcox added inline comments.Wed, Jun 5, 21:32
contrib/devtools/update-chainparams.py
11 ↗(On Diff #9124)

--rest=1 enables the node to accept HTTP RPC request (otherwise it rejects them). bitcoin-cli doesn't care.

44 ↗(On Diff #9124)

Good point.

jasonbcox updated this revision to Diff 9174.EditedWed, Jun 5, 22:00
  • Fixed a bug where calling update-chainparams.py from anything other than contrib/devtools would fail.
  • Used multiline strings for better readability
  • Improved code flow
  • Minor tweaks
jasonbcox edited the test plan for this revision. (Show Details)Wed, Jun 5, 22:02
Fabien requested changes to this revision.Thu, Jun 6, 07:28
Fabien added a subscriber: Fabien.
Fabien added inline comments.
contrib/devtools/update-chainparams.py
42 ↗(On Diff #9174)

If you use the python requests module, the whole thing becomes:

import requests
response = requests.get(url)

if response.status_code != requests.codes.ok:
    print("curl error status: {}".format(response.status_code))
    exit(1)

return response.json()

This is more portable but also adds a new dependency, so I leave that up to you.
Link to the python requests documentation.

46 ↗(On Diff #9174)

If the user calls update-chainparams.py -t -p 8332 it ends up changing the port silently.
You can remove the default port in argparse and if not set override it to 8332 or 18332 depending on testnet.

65 ↗(On Diff #9174)

If the block is not found curl would return status 0, and this will crash.

69 ↗(On Diff #9174)

The pythonic way:
chainParamsClass = 'TestNetParams' if args.testnet else 'MainParams'

82 ↗(On Diff #9174)

\w is not exactly what you want here. The file being utf-8 it can match a lot of characters, and even in ASCII it also includes the underscore char. I think [a-zA-Z0-9] is what you want here, maybe even only the lowercase chars.

This revision now requires changes to proceed.Thu, Jun 6, 07:28
jasonbcox marked 7 inline comments as done.Tue, Jun 11, 23:42
jasonbcox added inline comments.
contrib/devtools/update-chainparams.py
42 ↗(On Diff #9174)

I prefer to keep the dependencies to a minimum considering that this works and is still reasonably readable.

jasonbcox marked an inline comment as done.Tue, Jun 11, 23:43
jasonbcox updated this revision to Diff 9335.

Fixes according to feedback

jasonbcox edited the test plan for this revision. (Show Details)Tue, Jun 11, 23:45
jasonbcox planned changes to this revision.Wed, Jun 12, 15:40

Missed Amaury's comment regarding testing. This is a good idea and should be included.

jasonbcox updated this revision to Diff 9420.Fri, Jun 14, 00:01
  • Added unit tests
  • Improved test coverage
  • Added new unit tests as a new cmake target check-devtools
  • Fixed some bugs in the original script as a result of the new tests
jasonbcox edited the test plan for this revision. (Show Details)Fri, Jun 14, 00:03
deadalnix requested changes to this revision.Sat, Jun 15, 00:32
deadalnix added inline comments.
contrib/devtools/test/CMakeLists.txt
7 ↗(On Diff #9420)

This should at least run with check-all . Please check how other tests are setup and do the same.

This revision now requires changes to proceed.Sat, Jun 15, 00:32
jasonbcox updated this revision to Diff 9509.Tue, Jun 18, 00:55

Added create_script_test_suite to easily create non-ctest test suites. This automatically adds said script to check-all in
addition to creating check-devtools.

Fabien requested changes to this revision.Wed, Jun 19, 09:14
Fabien added inline comments.
cmake/modules/TestSuite.cmake
7 ↗(On Diff #9509)

I don't think this is a good solution:

  • the script is not run with Ctest, and does not display as part of the test suite
  • this is limited to script with no arguments
  • there is no dependency, so modifying the script won't regenerate the target

I suggest you use the same solution than test_runner.py or bitcoin-util-test.py (look at the end of test/CMakeLists.txt)

contrib/devtools/test/update-chainparams-test.py
68 ↗(On Diff #9509)

Why don't you want to catch exceptions from this code ?

74 ↗(On Diff #9509)

git diff -- :/src/chainparams.cpp, otherwise I get:

fatal: ambiguous argument ':/src/chainparams.cpp': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
75 ↗(On Diff #9509)

err is not used, you can remove it

83 ↗(On Diff #9509)

git checkout HEAD -- :/src/chainparams.cpp

contrib/devtools/update-chainparams.py
38 ↗(On Diff #9509)

Dito, err is not used

49 ↗(On Diff #9509)

Note to reviewers: this catches the case where the result is empty (-rest=1 is not set).

78 ↗(On Diff #9509)

Dito, err is not used.

This revision now requires changes to proceed.Wed, Jun 19, 09:14
jasonbcox added inline comments.Wed, Jun 19, 18:57
contrib/devtools/test/update-chainparams-test.py
68 ↗(On Diff #9509)

Exceptions are being re-raised, not ignored.

jasonbcox updated this revision to Diff 9541.Wed, Jun 19, 19:48

Fixed according to feedback

Fabien accepted this revision.Wed, Jun 19, 20:03
jasonbcox edited the test plan for this revision. (Show Details)Wed, Jun 19, 20:04