contrib: testgen: remove redundant base58 implementation #24576

1. 
   theStack commented at 10:38 pm on March 15, 2022: member

   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)
2. DrahtBot added the label Scripts and tools on Mar 15, 2022
3. 
   DrahtBot commented at 11:13 pm on March 16, 2022: member

   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.

4. laanwj added the label Tests on Mar 17, 2022
5. 
   laanwj commented at 10:35 am on April 4, 2022: member

   Code review and lightly tested ACK 94c25d47539cf09b6dcb9e5211e5abb7c103725a This is a non-controversial cleanup and makes the testgen commands easier to use.
6. in contrib/testgen/gen_key_io_test_vectors.py:12 in 94c25d4753 outdated

    5@@ -6,16 +6,21 @@
    6 Generate valid and invalid base58/bech32(m) address and private key test vectors.
    7 
    8 Usage:
    9-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py valid 70 > ../../src/test/data/key_io_valid.json
   10-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py invalid 70 > ../../src/test/data/key_io_invalid.json
   11+    ./gen_key_io_test_vectors.py valid 70 > ../../src/test/data/key_io_valid.json
   12+    ./gen_key_io_test_vectors.py invalid 70 > ../../src/test/data/key_io_invalid.json
   13 '''
   14 # 2012 Wladimir J. van der Laan
   

   ---

   ---

   

   laanwj commented at 10:39 am on April 4, 2022:

   Nit: could update this to the normal copyright header.

   ---

   

   theStack commented at 6:09 pm on April 5, 2022:

   Thanks, removed this line as well as the redundant MIT license information in the next line and updated the year in the header on top.
7. contrib: testgen: avoid need for manually setting PYTHONPATH 7d755bb31c
8. contrib: testgen: import OP_* constants from test framework 11c63e374d
9. scripted-diff: rename `chars` to `b58chars` in test_framework.address

   -BEGIN VERIFY SCRIPT-
   sed -i 's/chars/b58chars/g' ./test/functional/test_framework/address.py
   -END VERIFY SCRIPT-

   605fecfb66
10. contrib: testgen: use base58 methods from test framework 219d2c7ee1
11. test: throw `ValueError` for invalid base58 checksum 65c49ac750
12. in contrib/testgen/gen_key_io_test_vectors.py:115 in 94c25d4753 outdated

    108@@ -114,8 +109,10 @@ def is_valid(v):
    109     '''Check vector v for validity'''
    110     if len(set(v) - set(b58chars)) > 0:
    111         return is_valid_bech32(v)
    112-    result = b58decode_chk(v)
    113-    if result is None:
    114+    try:
    115+        payload, version = base58_to_byte(v)
    116+        result = bytes([version]) + payload
    117+    except AssertionError:  # thrown if checksum doesn't match
    

    ---

    ---

    

    laanwj commented at 11:48 am on April 5, 2022:

    Catching an AssertionError for expected error handling is a bit strange. Normally one’d use ValueError or such for invalid values.

    ---

    

    theStack commented at 6:10 pm on April 5, 2022:

    Agreed that AssertionError is too generic for this case, I added a commit which changes this to ValueError, as suggested.
13. theStack force-pushed on Apr 5, 2022
14. 
    laanwj commented at 11:57 am on April 6, 2022: member

    Code review ACK 65c49ac750ba39801b349d0a59c27471dfa9868e
15. fanquake merged this on Apr 6, 2022
16. fanquake closed this on Apr 6, 2022

    ---

17. theStack deleted the branch on Apr 6, 2022
18. sidhujag referenced this in commit 9862752109 on Apr 6, 2022
19. DrahtBot locked this on Apr 6, 2023

Contributors
theStack DrahtBot laanwj

Labels
Tests Scripts and tools