Bugfix & simplify bn2vch using int.to_bytes #18378

1. 
   sipa commented at 5:22 pm on March 18, 2020: member

   Alternative to #18374, fixing the incorrect padding added sometimes in bn2vch.

   Since we’re using Python 3.2+, a much simpler implementation of bn2vch is possible using int.to_bytes.

   This also adds a “functional” test for bn2vch, in a new “framework_test_script.py”, where the “framework_test_” prefix is intended for tests of the framework itself.

2. in test/functional/test_runner.py:225 in 07904b49c8 outdated

   221@@ -222,6 +222,7 @@
   222     'rpc_help.py',
   223     'feature_help.py',
   224     'feature_shutdown.py',
   225+    'framework_test.py',
   

   ---

   ---

   

   MarcoFalke commented at 6:07 pm on March 18, 2020:

   Not sure if it makes sense to add an unscoped general “framework test”. In the past we either assumed that the test framework is RIGHT(TM)(C) or added an inline unit test in the file that needed that part of the test framwork. For example,

   0$ git grep 'This is a sanity check of our testing fram'
   1test/functional/p2p_segwit.py:        # This is a sanity check of our testing framework.
   

   ---

   

   MarcoFalke commented at 6:10 pm on March 18, 2020:

   If you want to keep the file, it could make sense to rename it to framework_test_bn2vch or so.

   ---

   

   sipa commented at 6:20 pm on March 18, 2020:

   Yeah, an inline test would be fine, but it’s not clear where. This code is indirectly used from a number of locations I expect.

   Re separate file: That’s perhaps better; what about framework_test_script?

   ---

   

   sipa commented at 7:38 pm on March 18, 2020:

   Done.
3. fanquake added the label Tests on Mar 18, 2020
4. MarcoFalke requested review from jnewbery on Mar 18, 2020
5. sipa force-pushed on Mar 18, 2020
6. sipa force-pushed on Mar 18, 2020
7. Simplify bn2vch using int.to_bytes a3ad6459b7
8. Add bn2vch test to functional tests a733ad514a
9. sipa force-pushed on Mar 18, 2020
10. 
    jnewbery commented at 10:00 pm on March 18, 2020: member

    Concept ACK. The code was broken here: https://github.com/bitcoin/bitcoin/pull/17319/commits/f950ec25201e8ff7948be99ce3171f9700c4a8dc#diff-fb7bd6a727cfb726928c6f68fa8b3a80R20.
11. jnewbery approved
12. 
    jnewbery commented at 10:11 pm on March 18, 2020: member

    Tested ACK a733ad514a172a77309b84cbc6c81562bdf12e28.

    I cherry-picked just the test onto:

    • master before #17319: test passes
    • master after #17319: test fails
    • this PR: test passes

    Longer term, I think it’d be nicer if the tests for the test framework were unit tests within those files, because it doesn’t make much sense to me to have them as subclasses of the TestFramework object, and because it’s nice to have the test code next to the code it’s testing. However, that’s something that could be done separately.

13. 
    practicalswift commented at 10:21 pm on March 18, 2020: contributor

    Concept ACK
14. 
    sipa commented at 10:23 pm on March 18, 2020: member

    @jnewbery Yeah, that’s pretty ugly. Happy to take any concrete suggestions (in this PR or later).
15. 
    jnewbery commented at 10:58 pm on March 18, 2020: member

    Happy to take any concrete suggestions (in this PR or later).

    Let’s do it in a follow-up.

16. 
    laanwj commented at 8:34 am on March 19, 2020: member

    nice, ACK a733ad514a172a77309b84cbc6c81562bdf12e28
17. laanwj merged this on Mar 19, 2020
18. laanwj closed this on Mar 19, 2020

    ---

19. MarcoFalke referenced this in commit a66ba6d029 on Apr 30, 2020
20. sidhujag referenced this in commit 945eebe383 on May 2, 2020
21. Fabcien referenced this in commit d679709894 on Jan 9, 2021
22. DrahtBot locked this on Feb 15, 2022

Contributors
sipa MarcoFalke jnewbery practicalswift laanwj

Labels
Tests