Skip to content

Conversation

@theStack
Copy link
Contributor

This PR removes the redundant base58 implementation contrib/testgen/base58.py for the test generation script gen_key_io_test_vectors.py and uses the one from the test framework instead. Additionally, three other cleanups/improvements are done:

  • import script operator constants OP_* from test framework instead of manually defining them
  • add Python path to test framework directly in the script (via sys.path.append(...)) instead of needing the caller to specify PYTHONPATH=... on the command line (the same approach is done for the signet miner and the message capture scripts)
  • rename chars to b58chars in the test_framework.address module (is more explicit and makes the diff for the base58 replacement smaller)

@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #21279 (scripted-diff: Regenerate key_io data deterministically by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj
Copy link
Member

laanwj commented Apr 4, 2022

Code review and lightly tested ACK 94c25d47539cf09b6dcb9e5211e5abb7c103725a
This is a non-controversial cleanup and makes the testgen commands easier to use.

@theStack theStack force-pushed the 202203-contrib-various_testgen_improvements branch from 94c25d4 to 65c49ac Compare April 5, 2022 18:08
@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

Code review ACK 65c49ac

@fanquake fanquake merged commit 10f629e into bitcoin:master Apr 6, 2022
@theStack theStack deleted the 202203-contrib-various_testgen_improvements branch April 6, 2022 13:17
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 6, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants