Fix Base36 round-trip for empty, all-zero, and high-bit-set inputs (fixes #67) - #72
Open
youdie006 wants to merge 1 commit into
Open
Conversation
Base36.encode/decode route bytes through BigInteger, which loses fidelity two ways so decode(encode(x)) does not return x: (1) the empty/all-zero case maps to "0"/[0] instead of ""/[], compounding over a round trip ([] -> "0" -> [0,0]); (2) BigInteger.toByteArray() prepends a 0x00 two's-complement sign byte for any value whose top byte has the high bit set, which decode never strips ([0x80] -> "3k" -> [0x00,0x80]), affecting roughly half of all inputs. Both are reachable through the public Multibase API (k/K codes). Mirror the contract of this repo's own Base58 (empty -> empty, leading zeros preserved, no sign byte): encode emits "" for a zero value, and decode returns an empty array for empty input and strips a single leading 0x00 sign byte for a non-zero value. Fixes multiformats#67.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Base36.encode/decoderoute bytes throughBigInteger, which loses fidelity two ways, sodecode(encode(x))does not returnx:new BigInteger(1, allZeros).toString(36)yields"0"not"", andnew BigInteger("0", 36).toByteArray()yields[0]not an empty array. Each side adds a spurious byte/char and they compound over a round trip:[]->"0"->[0, 0].BigInteger.toByteArray()prepends a0x00two's-complement sign byte thatdecodenever strips:[0x80]->"3k"->[0x00, 0x80]. This affects roughly half of all inputs.Both are reachable through the public
MultibaseAPI, which routes theBase36/Base36Uppercodes (k/K) straight to these methods.Fix
Mirror the contract of this repo's own
Base58(empty -> empty, leading zeros preserved, no sign byte), keeping the existing leading-zero-prefix logic:encode: emit""for a zero value instead of"0".decode: returnnew byte[0]for empty input, and strip a single leading0x00sign byte fromtoByteArray()for a non-zero value.For a positive
BigInteger,toByteArray()has at most one leading0x00(the sign byte), and only when the top magnitude byte's high bit is set, so stripping one is always safe; genuine leading-zero bytes of the input are still carried by the separatezeroPrefixLengthpath.Tests
New
Base36Test(JUnit 5, matching the existing suite):{},{0},{0,0},{0x80},{0x01};"";0x00 ++ decode(body);The exhaustive check goes from 33027 of 65793 inputs failing (before) to 0 (after). Pre-existing
MultibaseTest(108) andMultibaseBadInputsTest(9) stay green; 121 tests pass total.Thanks to @hossman for the clear report and repro.
This change was prepared with AI assistance and reviewed by me before submission.