silentpayments: drop "empty key arrays must be NULL" requirement #1932

pull theStack wants to merge 2 commits into bitcoin-core:master from theStack:sp-keylists-drop-NULL-requirement changing 3 files +25 −22

theStack commented at 4:02 PM on September 7, 2026: contributor

Resolves #1930.

silentpayments: drop "empty key arrays must be NULL" requirement

Enforcing this doesn't have much value and is in the end just making
things slightly more inconvenient for the API user, forcing them to use
conditional constructs like `keys_size == 0 ? NULL : keys_ptrs` for the
call-sites, so removing it seems reasonable.

89e3a70662

in CHANGELOG.md:10 in 90a318baa0
```
   6 | @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   7 |  
   8 |  ## [Unreleased]
   9 |  
  10 | +#### Changed
```
real-or-random commented at 8:00 PM on September 7, 2026:

nit: I think it's a bug, strictly speaking, because the implementation doesn't match the docs.

#### Fixed
theStack commented at 9:16 PM on September 7, 2026:

Agree, done.
real-or-random added the label bug on Sep 7, 2026
real-or-random requested review from Copilot on Sep 7, 2026
?
copilot_work_started real-or-random

Copilot commented at 8:04 PM on September 7, 2026: none

🟢 Approval recommended

The implementation, tests, documentation, and linked issue requirements are consistent.

<details> <summary>Pull request overview</summary>

Aligns silent-payments key-array validation with the documented API by ignoring array pointers when their size is zero.

Changes:

Makes key counts authoritative for pointer validation.
Adds coverage for NULL and non-NULL empty arrays.
Documents the compatibility improvement.
</details>

<details> <summary>File summaries</summary>

File	Description
`src/modules/silentpayments/main_impl.h`	Updates key-array argument checks.
`src/modules/silentpayments/tests_impl.h`	Tests revised empty-array semantics.
`CHANGELOG.md`	Records the API behavior change.

</details>

<details> <summary>Review details</summary>

Files reviewed: 3/3 changed files
Comments generated: 0
Review effort level: Balanced
</details>

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a code-review agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review?tool=webui#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

real-or-random commented at 8:08 PM on September 7, 2026: contributor

@nymius want to review this? :)
changelog: mention dropped NULL requirement for silentpayments key arrays ad3688d1e5
theStack force-pushed on Sep 7, 2026
real-or-random approved
real-or-random commented at 9:06 AM on September 8, 2026: contributor

utACK ad3688d1e5a1b2afdbc613b2489e2fdb2ec4121d
nymius commented at 11:08 AM on September 8, 2026: contributor

ACK ad3688d1e5a1b2afdbc613b2489e2fdb2ec4121d

I've to implement the bindings on top to be sure, as there are chances the code requires the checks anyway because of type inference.
real-or-random merged this on Sep 8, 2026
real-or-random closed this on Sep 8, 2026
theStack deleted the branch on Sep 8, 2026

Contributors

Labels

Linked (view graph)

#1930 silentpayments: NULL requirement for empty key arrays (inconsistency between API docs and ARG_CHECKs)