Bugfix in bn2vch: avoid excessive padding#18374
Closed
sipa wants to merge 1 commit intobitcoin:masterfrom
Closed
Conversation
Member
Author
|
I'm happy to add a quick test case for this, but I don't see any obvious place to put a test of the test framework itself. Any suggestions? |
4 tasks
kallewoof
reviewed
Mar 18, 2020
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
maflcko
reviewed
Mar 18, 2020
| ext = bytearray() | ||
| if v.bit_length() > 0: | ||
| have_ext = (v.bit_length() & 0x07) == 0 | ||
| if v.bit_length() > 0 && (v.bit_length() & 0x07) == 0: |
Member
There was a problem hiding this comment.
Suggested change
| if v.bit_length() > 0 && (v.bit_length() & 0x07) == 0: | |
| if v.bit_length() > 0 and (v.bit_length() & 0x07) == 0: |
Member
Author
|
Closing in favor of #18378. |
laanwj
added a commit
that referenced
this pull request
Mar 19, 2020
a733ad5 Add bn2vch test to functional tests (Pieter Wuille) a3ad645 Simplify bn2vch using int.to_bytes (Pieter Wuille) Pull request description: 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. ACKs for top commit: laanwj: nice, ACK a733ad5 jnewbery: Tested ACK a733ad5. Tree-SHA512: aeacc4e7fd84279023d38e8b4a5175fb16d7b3a7f93c61b9dcb59cd9927547732983c76f28564b62e37088399fc0121b38a514d73b0ea38b3983836539e9ca90
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It seems #17319 introduced an (unobserved) bug in the test framework in
bn2vch: a zero padding byte was added for every nonzero number, even when the top byte was <= 0x7F. Fix this.