BIP352: fix byte unpacking and avoid mutable default argument #1916

1. 
   fifalodm commented at 3:42 pm on August 8, 2025: none

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

2. Update bitcoin_utils.py c9f989b05e
3. Update reference.py 948f137345
4. jonatack added the label Proposed BIP modification on Aug 8, 2025
5. 
   murchandamus commented at 9:31 pm on August 18, 2025: contributor

   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?
6. 
   fifalodm commented at 8:46 am on August 19, 2025: none

   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.

7. 
   jonatack commented at 4:17 pm on August 19, 2025: member

   cc BIP authors @RubenSomsen @josibake for feedback
8. jonatack assigned RubenSomsen on Aug 19, 2025
9. jonatack added the label Pending acceptance on Aug 19, 2025
10. 
    josibake commented at 10:21 am on September 2, 2025: member

    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.

11. 
    murchandamus commented at 10:37 pm on September 2, 2025: contributor

    I’m closing this, because the proposed change was rejected by the BIP authors.
12. murchandamus closed this on Sep 2, 2025

    ---

13. jonatack unassigned RubenSomsen on Sep 2, 2025

Contributors
fifalodm murchandamus jonatack josibake

Labels
Proposed BIP modification Pending acceptance