Page MenuHomePhabricator

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

Authored by jasonbcox on Jun 4 2019, 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 updated this revision to Diff 9119.Jun 4 2019, 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.Jun 4 2019, 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)Jun 4 2019, 16:13
jasonbcox edited the summary of this revision. (Show Details)Jun 4 2019, 16:32
jasonbcox updated this revision to Diff 9124.Jun 4 2019, 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.Jun 4 2019, 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)Jun 4 2019, 19:23
deadalnix requested changes to this revision.Jun 5 2019, 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.Jun 5 2019, 01:30
jasonbcox added inline comments.Jun 5 2019, 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.EditedJun 5 2019, 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)Jun 5 2019, 22:02
Fabien requested changes to this revision.Jun 6 2019, 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.Jun 6 2019, 07:28
jasonbcox marked 7 inline comments as done.Jun 11 2019, 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 updated this revision to Diff 9335.Jun 11 2019, 23:43
jasonbcox marked an inline comment as done.

Fixes according to feedback

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

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

jasonbcox updated this revision to Diff 9420.Jun 14 2019, 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)Jun 14 2019, 00:03
deadalnix requested changes to this revision.Jun 15 2019, 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.Jun 15 2019, 00:32
jasonbcox updated this revision to Diff 9509.Jun 18 2019, 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.Jun 19 2019, 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.Jun 19 2019, 09:14
jasonbcox added inline comments.Jun 19 2019, 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.Jun 19 2019, 19:48

Fixed according to feedback

Fabien accepted this revision.Jun 19 2019, 20:03
jasonbcox edited the test plan for this revision. (Show Details)Jun 19 2019, 20:04
deadalnix requested changes to this revision.Jun 27 2019, 01:04

Rather than grepping and replacing within the script, you could simply generate a header that is 100% generated by the script and defines a few constants that then get used in the chainparam code. You'll get a more robust design and people won't get merge conflicts with a bot.

I'm also wondering how doable it is to factor the RPC fetching code with the one of the test framework, as clearly they do the same, but that's a bit out of scope for this patch. The ad hoc thing should do it for now.

contrib/devtools/test/update-chainparams-test.py
65

This is a sign you wire a ton of dependencies in. This makes your code fragile. Why does the code who fetch from the node and deduce infos would depend on the git setup of the project?

You need to read your own code as you go. It is telling you what you need to do - or at least what's wrong with hat you are doing and need addressing. This has no reason to be manipulating source control. For instance, if the file is moved, there are no reason for this test to break because the path has been updated everywhere but here.

This revision now requires changes to proceed.Jun 27 2019, 01:04
jasonbcox abandoned this revision.Jul 10 2019, 18:25

Abandoning in favor of D3502