Bugfix & simplify bn2vch using int.to_bytes #18378

pull sipa wants to merge 2 commits into bitcoin:master from sipa:202003_simple_bn2vch changing 3 files +54 −33

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

$ git grep 'This is a sanity check of our testing fram' test/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.
fanquake added the label Tests on Mar 18, 2020
MarcoFalke requested review from jnewbery on Mar 18, 2020
sipa force-pushed on Mar 18, 2020
sipa force-pushed on Mar 18, 2020
Simplify bn2vch using int.to_bytes a3ad6459b7
Add bn2vch test to functional tests a733ad514a
sipa force-pushed on Mar 18, 2020
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.
jnewbery approved
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.
practicalswift commented at 10:21 PM on March 18, 2020: contributor

Concept ACK
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).
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.
laanwj commented at 8:34 AM on March 19, 2020: member

nice, ACK a733ad514a172a77309b84cbc6c81562bdf12e28
laanwj merged this on Mar 19, 2020
laanwj closed this on Mar 19, 2020
MarcoFalke referenced this in commit a66ba6d029 on Apr 30, 2020
sidhujag referenced this in commit 945eebe383 on May 2, 2020
Fabcien referenced this in commit d679709894 on Jan 9, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18374 Bugfix in bn2vch: avoid excessive padding #18576 test: use unittest for test_framework unit testing