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.

test: Use FlatSigningProvider in SetupDummyInputs

Instead of using FillableSigningProvider, use a FlatSigningProvider.

703252bf5d

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

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:

#35955 (wallet: remove orphaned GetAffectedKeys and LegacyScriptPubKeyMan declarations by laxmanacharya8)
#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.

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. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29456873744/job/87491882670</sub> <sub>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).</sub>

<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

fuzz: Remove use of FillableSigningProvider 5dedc50f53

tests: Replace FillableSigningProvider with FlatSigningProvider

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

de0afe2dee

bitcoin-tx: Replace FillableSigningProvider with FlatSigningProvider

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

cfdb2cece7

achow101 force-pushed on Jul 18, 2026

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.

64b4a438f6

doc: Remove reference to FillableSigningProvider 632e42a12f

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

 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

 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

 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

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

 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

 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