Skip to content

Fix base58 encoding losing leading zero bytes - #48

Open
sumanjeet0012 wants to merge 1 commit into
multiformats:masterfrom
sumanjeet0012:fix-issue-35-leading-zeros
Open

Fix base58 encoding losing leading zero bytes#48
sumanjeet0012 wants to merge 1 commit into
multiformats:masterfrom
sumanjeet0012:fix-issue-35-leading-zeros

Conversation

@sumanjeet0012

Copy link
Copy Markdown
Contributor

Fixes #35

This PR fixes the issue where BaseStringConverter silently drops leading \x00 bytes during encoding and decoding for integer-based encodings (like base58btc). It now properly counts and preserves them by utilizing the encoding alphabet's zero character.

Changes made:

  • Modified BaseStringConverter.encode() and decode() to manually handle leading zero bytes.
  • Updated Base16StringConverter.decode() to use bytes.fromhex() directly to bypass the zero-counting logic, as base16 operates byte-by-byte.
  • Added comprehensive tests for base58btc, base58flickr, base32z, base36, base10, base8, and base2 with leading zero byte strings to ensure correct round-tripping.

All 287 tests pass successfully.

@acul71 acul71 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (maintainer) — Request changes

Thanks for tackling #35. The diagnosis is right, and non-all-zero leading-zero payloads round-trip correctly. I’m still requesting changes and prefer overlapping PR #55 for landing this fix.

Prefer #55

  • #55 fixes the same BaseStringConverter leading-zero bug, handles all-zeros and empty inputs, adds tests/test_roundtrip.py across all encodings, and has green CI.
  • #48 currently has no CI checks reported on fix-issue-35-leading-zeros.
  • Please treat #55 as the preferred merge vehicle. Leaving this PR open (not closing) for reference / coordination.

Blockers

  1. Missing newsfragment — add newsfragments/35.bugfix.rst (user-facing ReST + trailing newline). Mandatory for approval on this track.
  2. All-zeros off-by-onesuper().encode(0) already emits the zero digit, then leading zeros are prepended again. Locally: encode("base58btc", b"\x00")z11 (expected z1); decode yields an extra \x00. Same pattern for other integer bases.
  3. Empty inputencode("base58btc", b"")z1 / decodes to b"\x00". Needs an empty-input special case (as in #55).

Other notes

  • Branch is 6 behind / 1 ahead of origin/master; dry-run merge was clean (no conflicts) but should be synced if this PR continues.
  • New fixtures only cover \x00\x00 + non-zero payload; they don’t catch the all-zeros/empty bugs.
  • Local make lint, typecheck, test (287 passed), docs-ci all passed on the PR worktree — but CI absence on GitHub remains a concern.
  • Minor: parameter name bytes shadows the builtin; hasattr(..., "decode") is weaker than #55’s isinstance + rename.

Bottom line: Request changes — missing newsfragment, incomplete edge cases, no CI, superseded by stronger #55. Do not merge #48 as-is; prefer #55.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Base58 encoding loses leading zero bytes

2 participants