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
updateassumevalid
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6140
Build 10328: Bitcoin ABC Buildbot (legacy)
Build 10327: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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)

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)
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

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

Also, multiline stings are great.

44

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
contrib/devtools/update-chainparams.py
11

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

44

Good point.

  • 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
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 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.

Fixes according to feedback

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

  • 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
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

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
contrib/devtools/test/update-chainparams-test.py
68 ↗(On Diff #9509)

Exceptions are being re-raised, not ignored.

Fixed according to feedback

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 ↗(On Diff #9541)

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