#35733 sign: Remove FillableSigningProvider
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35733 sign: Remove FillableSigningProvider | Needs rebase | 0 | Strong | 86 + 104 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Removes the `FillableSigningProvider` class across the codebase, inlining its remaining functionality into `LegacyDataSPKM` where it is still needed for legacy wallet migration. Call sites in tests and `bitcoin-tx` are converted to use `FlatSigningProvider` with explicit key and script population. A small safeguard is also added to `SignStep` to avoid signing `SCRIPTHASH` redeem scripts exceeding 520 bytes.
Problem: Following the removal of the legacy wallet, `FillableSigningProvider` remained as an unnecessary and confusing abstraction in the signing subsystem.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Agreement
Strong
substantive support, no open objection (vicjuma, jeanpablojp, rkrux)
Thread notes: First record for this PR. jeanpablojp noted three correctness issues (producing consensus-invalid P2SH transactions and two fuzz test discrepancies), all of which achow101 fixed.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, correctness | signing P2SH redeemScripts over 520 bytes produces consensus-invalid transactions with Push value size limit exceeded | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-08-25: 'This branch fixes P2WSH, which now makes it into the mempool, and also signs P2SH, producing a transaction that is consensus-invalid with `Push value size limit exceeded`.' Status from: 2026-08-26: achow101: 'I added a commit to enforce this in signing.' |
| jeanpablojp (contributor) | objection, correctness | LoadCScript returning true for scripts over 520 bytes causes an accounting mismatch in spkm_migration fuzz testing | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-08-25: 'For a script over 520 bytes `LoadCScript` returns true without storing it, where `AddCScript` returned false. So `added_script` counts a script that was never stored, and the `>=` branch runs instead of the exact-equality one.' Status from: 2026-08-26: achow101: 'Fixed' |
| jeanpablojp (contributor) | objection, correctness | script_sign fuzz test fails to look up P2WPKH scripts previously added implicitly for compressed keys | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-08-25: '`AddKey(k)` also answered `GetPubKey()` and, for compressed keys, inserted the P2WPKH script P2SH-P2WPKH needs. The current `script_sign` corpus finds both, and with only `provider.keys` those lookups start failing.' Status from: 2026-08-26: achow101: 'Done' |
Support:
- vicjuma (contributor) (ACK, 2026-08-10): ACK for the bitcoin-tx changes. Will try to review the remaining parts
- jeanpablojp (contributor) (Approach ACK, 2026-08-25): Built and ran the tests.
- rkrux (member) (Concept ACK, 2026-08-26): to containerise the codepath related to legacy wallets while discouraging usage of this seemingly outdated signing provider.
Participants:
- vicjuma (contributor, support, 6 statements 2026-08-10 to 2026-08-10): tested and ACKed the bitcoin-tx changes
- jeanpablojp (contributor, objection, 4 statements 2026-08-25 to 2026-08-25): Approach ACKed but raised three issues regarding consensus validity and fuzz test regressions, all resolved
- rkrux (member, support, 1 statements 2026-08-26 to 2026-08-26): Concept ACKed containerizing legacy wallet code and removing the outdated provider
Files
191 lines under test/bench/ci.
- src/script/signingprovider.cpp +0/-109
- src/script/signingprovider.h +0/-74
- src/wallet/scriptpubkeyman.cpp +62/-10
- src/test/transaction_tests.cpp +29/-23
- src/test/fuzz/key.cpp +14/-20
- src/test/script_p2sh_tests.cpp +16/-13
- src/bitcoin-tx.cpp +10/-4
- src/test/script_tests.cpp +7/-6
- src/wallet/scriptpubkeyman.h +10/-2
- src/wallet/test/wallet_tests.cpp +7/-5
- src/test/fuzz/script_sign.cpp +8/-2
- src/test/txvalidationcache_tests.cpp +4/-3
- src/wallet/test/fuzz/scriptpubkeyman.cpp +4/-2
- src/test/orphanage_tests.cpp +3/-2
- src/test/util/setup_common.cpp +3/-2
- src/wallet/test/spend_tests.cpp +3/-2
- src/test/multisig_tests.cpp +2/-2
- src/test/util/transaction_utils.cpp +2/-2
- doc/developer-notes.md +1/-2
- src/script/sign.cpp +3/-0
- src/test/util/transaction_utils.h +1/-2
- src/bench/ccoins_caching.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
Removes the obsolete FillableSigningProvider class left over from legacy wallet architecture and inlines its remaining logic into LegacyDataSPKM for legacy wallet migration. Bitcoin-tx and test call sites are updated to use FlatSigningProvider. While an internal cleanup with low strategic urgency, it carries solid support from reviewers and the author has resolved technical concerns regarding 520-byte P2SH script limits.
Data
thread read through 2026-08-26 (14 statements), head b68a3837ec
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Needs rebase
Categories
Wallet
P3 · cleanup
Confinies legacy wallet signing provider logic to LegacyDataSPKM following legacy wallet removal. Rkrux notes it serves to 'containerise the codepath related to legacy wallets while discouraging usage of this seemingly outdated signing provider'.
Membership: Modifies src/wallet/scriptpubkeyman.cpp (+62/-10) and .h (+10/-2) to inline FillableSigningProvider functionality into LegacyDataSPKM.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1