Skip to content

SP fixes#133

Closed
notTanveer wants to merge 2 commits into
diybitcoinhardware:feat/sp-phase2from
notTanveer:perf-sp
Closed

SP fixes#133
notTanveer wants to merge 2 commits into
diybitcoinhardware:feat/sp-phase2from
notTanveer:perf-sp

Conversation

@notTanveer

Copy link
Copy Markdown

Four targeted fixes from the post-implementation code audit:

  • SPOutputScope.write_to: add if version == 2 guard so sp_data /
    sp_label are never written into a PSBTv0 (or version=None) stream.
    Without it the round-trip raises PSBTError on parse.

  • _sign_with_sp: remove silent except SPFieldError: continue. We only
    reach share computation when we've already resolved the private key for
    that input; swallowing the error hides RNG or key-op failures and
    returns success with an incomplete PSBT.

  • create_outputs: replace signing_keys list + sum() with a running
    accumulator, eliminating the O(n) bigint allocation before the sum.

  • derive_silent_payment_outputs: drop the dead shared_secret parameter
    (no caller ever passed it; body always fell through to = ecdh_share).

  • use the already present NUMS_VALUE.

Four targeted fixes from the post-implementation code audit:

- SPOutputScope.write_to: add `if version == 2` guard so sp_data /
  sp_label are never written into a PSBTv0 (or version=None) stream.
  Without it the round-trip raises PSBTError on parse.

- _sign_with_sp: remove silent `except SPFieldError: continue`. We only
  reach share computation when we've already resolved the private key for
  that input; swallowing the error hides RNG or key-op failures and
  returns success with an incomplete PSBT.

- create_outputs: replace signing_keys list + sum() with a running
  accumulator, eliminating the O(n) bigint allocation before the sum.

- derive_silent_payment_outputs: drop the dead `shared_secret` parameter
  (no caller ever passed it; body always fell through to `= ecdh_share`).
NUMS_PUBKEY is already defined in ec.py. NUMS_H was a redundant copy
of the same 32-byte value, with its own unhexlify import. Replace both
with a module-level _NUMS_XONLY = ec.NUMS_PUBKEY.xonly() so the
canonical definition lives in one place.
@odudex

odudex commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

This PR is now covered by #134

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.

2 participants