sign: Remove FillableSigningProvider #35733

achow101 commented at 10:53 PM on July 15, 2026: member

FillableSigningProvider was initially extracted from the wallet in order to produce the SigningProvider encapsulation system that we use today for the wallet and for providing key material to signing. It's primary user was the legacy wallet, but it was also being used as a convenience in a few other places. Since the legacy wallet is removed, there is no need to keep it around as a separate class. Instead, the things that it was doing can be inlined into LegacyDataSPKM where the functionality is actually being used to support migrating legacy wallets.

The remaining uses are tests and the bitcoin-tx tool, some of which were relying on the implicit addition of segwit scripts when adding a key. However, these can also use FlatSigningProvider with explicitly adding the segwit scripts when necessary, and have thusly been converted to do so.

DrahtBot commented at 10:53 PM on July 15, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35733.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	vicjuma, rkrux
Approach ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36116 (iwyu: Fix warnings in src/rpc and treat them as errors by hebasto)
#34729 (Reduce log noise by ajtowns)
#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

SignSignature(keystore, CTransaction(txFrom), txTo, 0, SIGHASH_ALL, dummy_c) in src/test/script_tests.cpp

2026-08-26 18:13:43

DrahtBot added the label CI failed on Jul 15, 2026

DrahtBot commented at 11:51 PM on July 15, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29456873744/job/87491882670 LLM reason (✨ experimental): CI failed because the build broke under clang with -Werror,-Wthread-safety-analysis (missing exclusive lock on cs_KeyStore in scriptpubkeyman.cpp).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

achow101 force-pushed on Jul 17, 2026

achow101 force-pushed on Jul 18, 2026

DrahtBot removed the label CI failed on Jul 18, 2026

in src/bitcoin-tx.cpp:604 in 632e42a12f outdated

 599 | @@ -600,7 +600,13 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
 600 |          if (!key.IsValid()) {
 601 |              throw std::runtime_error("privatekey not valid");
 602 |          }
 603 | -        tempKeystore.AddKey(key);
 604 | +        CPubKey pubkey = key.GetPubKey();
 605 | +        tempKeystore.keys.emplace(pubkey.GetID(), key);

vicjuma commented at 9:22 PM on August 10, 2026:

The key path works correctly. A helper function would maybe be handy same as the AddKey one but since we are only using it in a single place, I guess expanding it here may be fine.

bitcoin-tx testing

Creating unsigned transaction

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ TX=$(./bitcoin-tx -regtest -create \
     in=7dc357e7ba70b0b503c9a12f4a293add0b1d230e08de3cad83e142737746c4b8:1 \
     outaddr=0.9:mmhnpJgEDuPxTmw6Nuu5Ms2xmP21n6XoCi)
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$

Signing the transaction

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ TX=$(./bitcoin-tx -regtest "$TX" \
     set=prevtxs:"[{\"txid\":\"7dc357e7ba70b0b503c9a12f4a293add0b1d230e08de3cad83e142737746c4b8\",\"vout\":1,\"scriptPubKey\":\"76a91443dd6b08400c8e763c5e1b8520957e132cac817088ac\",\"amount\":1}]" \
     set=privatekeys:"[\"cNPgXXtoNEkhZd6txFEPQvWMF8gfvyG15JnYxgtrbQgiesHPwERA\"]" \
     sign=ALL)

Confirming

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest testmempoolaccept "[\"$TX\"]" 0
[
  {
    "txid": "96aba7a976963a610c82d8fe5ba0f5da1837d64e875127fc74b3253c2de1b9d9",
    "wtxid": "96aba7a976963a610c82d8fe5ba0f5da1837d64e875127fc74b3253c2de1b9d9",
    "allowed": true,
    "vsize": 191,
    "fees": {
      "base": 0.10000000
    }
  }
]

The values used were generated from bitcoin 24.0 binary from the commands

in src/bitcoin-tx.cpp:662 in 632e42a12f outdated

 664 | @@ -659,12 +665,12 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
 665 |                  UniValue v = prevOut["redeemScript"];
 666 |                  std::vector<unsigned char> rsData(ParseHexUV(v, "redeemScript"));
 667 |                  CScript redeemScript(rsData.begin(), rsData.end());
 668 | -                tempKeystore.AddCScript(redeemScript);

vicjuma commented at 9:32 PM on August 10, 2026:

Following this function, the maximum script element size condition is being checked redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE

in src/bitcoin-tx.cpp:668 in 632e42a12f outdated

 664 | @@ -659,12 +665,12 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
 665 |                  UniValue v = prevOut["redeemScript"];
 666 |                  std::vector<unsigned char> rsData(ParseHexUV(v, "redeemScript"));
 667 |                  CScript redeemScript(rsData.begin(), rsData.end());
 668 | -                tempKeystore.AddCScript(redeemScript);
 669 | +                tempKeystore.scripts.emplace(CScriptID(redeemScript), redeemScript);

vicjuma commented at 9:59 PM on August 10, 2026:

It was thoughtful to remove the check since pay-to-witness-script-hash doesn't share the same MAX_SCRIPT_ELEMENT_SIZE limit as the pay-to-script-hash.

The script path works correctly.

bitcoin-tx testing

Creating unsigned transaction

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ TX=$(./bitcoin-tx -regtest -create \
  in=f4e3e878eefc814436f3cf27752bb1896934dcd8fa8663ffd8187e9358d0c03b:0 \
  outaddr=0.9:mwbmunN2ojMDLGWwZJuqPNX3dfyuvNqheF)
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$

Signing the transaction

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ TX=$(./bitcoin-tx -regtest "$TX" \
  set=prevtxs:"[{\"txid\":\"f4e3e878eefc814436f3cf27752bb1896934dcd8fa8663ffd8187e9358d0c03b\",\"vout\":0,\"scriptPubKey\":\"a91417c74210b483bf6dd004660b05b356b6c3aa12dc87\",\"redeemScript\":\"522103804969501e96ed27704e3d82bef2b5ef04a53d80683ea1a4dbbb9d2e63b7e09921034c988b448e0fa6a1fcc09f33a20abf5ab0ca3e393fa717bb251493dd2819412752ae\",\"amount\":1.00000000}]" \
  set=privatekeys:"[\"cTzVvTMsbz75L9mF15FdM6AZWe22EXstSoQGhBGNMjYRjm28N7Yx\",\"cNc7X8u7KCubwor6rsuSMFdNCCEwK2qSkbHv5WFTPDWmBdRNWHgG\"]" \
  sign=ALL)

Confirming

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest testmempoolaccept "[\"$TX\"]" 0
[
  {
    "txid": "6286887e90dfd4aebcbdec8d1949593ce46ed6b0d10a03e8f84db6fe7d52a914",
    "wtxid": "6286887e90dfd4aebcbdec8d1949593ce46ed6b0d10a03e8f84db6fe7d52a914",
    "allowed": true,
    "vsize": 302,
    "fees": {
      "base": 0.10000000
    }
  }
]

The values used were generated from bitcoin 24.0 binary from the commands

jeanpablojp commented at 11:28 PM on August 25, 2026:

AddCScript rejected scripts over 520 bytes for both P2SH and P2WSH, but the limit only applies to the script the scriptSig has to push. With a 1-of-16 multisig of 547 bytes, master leaves both inputs unsigned. 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. Worth keeping the limit for the script the scriptSig pushes? Neither case seems to have a vector in bitcoin-util-test.json.

achow101 commented at 6:13 PM on August 26, 2026:

I added a commit to enforce this in signing.

in src/bitcoin-tx.cpp:593 in 632e42a12f outdated

 589 | @@ -590,7 +590,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
 590 |  
 591 |      if (!registers.contains("privatekeys"))
 592 |          throw std::runtime_error("privatekeys register variable must be set.");
 593 | -    FillableSigningProvider tempKeystore;
 594 | +    FlatSigningProvider tempKeystore;

vicjuma commented at 10:14 PM on August 10, 2026:

appropriate for the self-contained bitcoin-tx

in src/bitcoin-tx.cpp:673 in 632e42a12f outdated

 670 |              }
 671 |          }
 672 |      }
 673 |  
 674 | -    const FillableSigningProvider& keystore = tempKeystore;
 675 | +    const FlatSigningProvider& keystore = tempKeystore;

vicjuma commented at 10:14 PM on August 10, 2026:

appropriate for the self-contained bitcoin-tx

vicjuma commented at 10:24 PM on August 10, 2026: contributor

ACK for the bitcoin-tx changes. Will try to review the remaining parts

DrahtBot added the label Needs rebase on Aug 18, 2026

achow101 force-pushed on Aug 19, 2026

DrahtBot removed the label Needs rebase on Aug 19, 2026

jeanpablojp commented at 11:28 PM on August 25, 2026: contributor

Approach ACK

Built and ran the tests.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:334 in 03969968a7

 330 | @@ -331,7 +331,7 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration)
 331 |                  }
 332 |                  if (pubkeys.size() < num_keys) return;
 333 |                  CScript multisig_script{GetScriptForMultisig(num_keys, pubkeys)};
 334 | -                if (!legacy_data.HaveCScript(CScriptID(multisig_script)) && legacy_data.AddCScript(multisig_script)) {
 335 | +                if (!legacy_data.HaveCScript(CScriptID(multisig_script)) && legacy_data.LoadCScript(multisig_script)) {

jeanpablojp commented at 11:28 PM on August 25, 2026:

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. 16 compressed keys already reach 547 bytes.

achow101 commented at 6:13 PM on August 26, 2026:

Fixed

in src/test/fuzz/script_sign.cpp:85 in 03969968a7

  78 | @@ -79,10 +79,10 @@ FUZZ_TARGET(script_sign, .init = initialize_script_sign)
  79 |          signature_data_1.MergeSignatureData(signature_data_2);
  80 |      }
  81 |  
  82 | -    FillableSigningProvider provider;
  83 | +    FlatSigningProvider provider;
  84 |      CKey k = ConsumePrivateKey(fuzzed_data_provider);
  85 |      if (k.IsValid()) {
  86 | -        provider.AddKey(k);
  87 | +        provider.keys.emplace(k.GetPubKey().GetID(), k);

jeanpablojp commented at 11:28 PM on August 25, 2026:

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. Could the explicit insertions bitcoin-tx does be kept here too?

achow101 commented at 6:13 PM on August 26, 2026:

Done

rkrux commented at 2:09 PM on August 26, 2026: contributor

Concept ACK 03969968a7dd517c0ae7ba31918ff5244ccb3839 to containerise the codepath related to legacy wallets while discouraging usage of this seemingly outdated signing provider.

sign: Do not sign SCRIPTHASH redeemScripts > 520 bytes cf0a680f52

test: Use FlatSigningProvider in SetupDummyInputs

Instead of using FillableSigningProvider, use a FlatSigningProvider.

34cf766ca0

fuzz: Remove use of FillableSigningProvider 1a98da5032

tests: Replace FillableSigningProvider with FlatSigningProvider

FillableSigningProvider is not a necessary part of the tests and
FlatSigningProvider is sufficient.

a54f84c022

bitcoin-tx: Replace FillableSigningProvider with FlatSigningProvider

bitcoin-tx does not need to use FillableSigningProvider, it can use
FlatSigningProvider and inline ImplicitlyLearnRelatedKeyScripts.

7d132abf00

wallet: Combine FillableSigningProvider into LegacyDataSPKM

The only real user of FillableSigningProvider is LegacyDataSPKM. Instead
of having it be a separate class which LegacyDataSPKM subclasses, move
the implementation directly into LegacyDataSPKM and have LegacyDataSPKM
be a subclass of SigningProvider.

976bd33a8b

doc: Remove reference to FillableSigningProvider 814fdcba23

achow101 force-pushed on Aug 26, 2026