Skip to content

Conversation

fifalodm
Copy link

@fifalodm fifalodm commented Aug 8, 2025

  • Fixed deser_compact_size to correctly unpack the first byte instead of reading twice.

  • Updated scanning to use Optional[Dict] with safe default (None{}) to prevent mutable default argument issues.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

I’m not familiar enough with Python to know at first glance—is this fixing an actual bug in the reference implementation or is this just a stylistic improvement?

@fifalodm
Copy link
Author

Thanks for the review!

These are actual bug fixes, not just stylistic changes:

  1. deser_compact_size bug
    The old code did b = f.read(1) and then immediately called struct.unpack("<B", f.read(1))[0].
    That skipped the first byte and unpacked the next one.

    • For values like 0xfd/0xfe/0xff (multi-byte varints), the prefix byte was lost and parsing went out of sync.
    • Even for small values (≤252), the stream advanced one byte too far.

    The fix correctly unpacks the already-read b, so the prefix is handled properly.

  2. Mutable default argument in scanning
    Using {} as a default creates a single shared dict across calls. This can leak state between function calls.
    Switching to labels: Optional[Dict[str, str]] = None and initializing inside the function avoids that Python pitfall.

So both changes address correctness issues, not just style.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

cc BIP authors @RubenSomsen @josibake for feedback

@jonatack jonatack added the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Aug 19, 2025
@josibake
Copy link
Member

josibake commented Sep 2, 2025

Hi @fifalodm , thanks for taking a look!

Its a NACK from me on this PR only because changing reference code and code related to the test vectors can (rightly) raise a lot of alarms for downstream projects using these test vectors.

It's my belief that this python code is correct enough for the use case, and the goal here is not to write best-in-class python code, but rather something that is readable enough when comparing to the specification in the BIP.

If you are interested in contributing to silent payments, there are plenty of other places where we need contributors/reviewers and I'd be more than happy to help get you situated! Don't hesitate to reach out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified Proposed BIP modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants