PSBT key path cleanups #13723

pull sipa wants to merge 11 commits into bitcoin:master from sipa:201807_key_origin_provider changing 11 files +232 −138
  1. sipa commented at 7:15 AM on July 20, 2018: member

    This PR adds "key origin" (master fingeprint + key path) information to what is exposed from SigningProviders, allowing this information to be used by the generic PSBT code instead of having the RPC pull it directly from the wallet.

    This is also a preparation to having PSBT interact with output descriptors, which can then directly expose key origin information for the scripts they generate.

  2. sipa force-pushed on Jul 20, 2018
  3. in src/script/sign.cpp:628 in dc3ad0d3d9 outdated 
     623 | @@ -624,3 +624,8 @@ bool PublicOnlySigningProvider::GetPubKey(const CKeyID &address, CPubKey& pubkey
 624 |  {
 625 |      return m_provider->GetPubKey(address, pubkey);
 626 |  }
 627 | +
 628 | +bool PublicOnlySigningProvider::GetKeyOrigin(const CKeyID &keyid, KeyOriginInfo& info) const
    promag commented at 9:58 AM on July 20, 2018:

    Commit "Make SigningProvider expose key origin information"

    nit CKeyID& keyid.

    sipa commented at 6:54 PM on July 20, 2018:

    done

  4. in src/wallet/wallet.cpp:4439 in dc3ad0d3d9 outdated 
    4377 | @@ -4378,4 +4378,3 @@ void CWallet::LearnAllRelatedScripts(const CPubKey& key)
4378 |      // OutputType::P2SH_SEGWIT always adds all necessary scripts for all types.
4379 |      LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);
4380 |  }
4381 | -
    promag commented at 9:59 AM on July 20, 2018:

    Commit "Make SigningProvider expose key origin information"

    nit, remove.

    sipa commented at 6:54 PM on July 20, 2018:

    done

  5. in src/script/sign.h:51 in dc3ad0d3d9 outdated 
      47 | @@ -47,6 +48,7 @@ class PublicOnlySigningProvider : public SigningProvider
  48 |      PublicOnlySigningProvider(const SigningProvider* provider) : m_provider(provider) {}
  49 |      bool GetCScript(const CScriptID &scriptid, CScript& script) const;
  50 |      bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const;
  51 | +    bool GetKeyOrigin(const CKeyID &address, KeyOriginInfo& info) const;
    promag commented at 9:59 AM on July 20, 2018:

    Commit "Make SigningProvider expose key origin information"

    nit, CKeyID& address.

    sipa commented at 6:54 PM on July 20, 2018:

    done

  6. in src/script/sign.cpp:653 in f6631bf2ca outdated 
     632 |  {
 633 | +    if (m_hide_secret) return false;
 634 | +    return m_provider->GetKey(keyid, key);
 635 | +}
 636 | +
 637 | +bool HidingSigningProvider::GetKeyOrigin(const CKeyID &keyid, KeyOriginInfo& info) const
    promag commented at 10:01 AM on July 20, 2018:

    Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

    nit, CKeyID& keyid.

    sipa commented at 6:54 PM on July 20, 2018:

    done

  7. in src/script/sign.h:51 in f6631bf2ca outdated 
      51 | -    PublicOnlySigningProvider(const SigningProvider* provider) : m_provider(provider) {}
  52 | -    bool GetCScript(const CScriptID &scriptid, CScript& script) const;
  53 | -    bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const;
  54 | -    bool GetKeyOrigin(const CKeyID &address, KeyOriginInfo& info) const;
  55 | +    HidingSigningProvider(const SigningProvider* provider, bool hide_secret, bool hide_origin) : m_hide_secret(hide_secret), m_hide_origin(hide_origin), m_provider(provider) {}
  56 | +    bool GetCScript(const CScriptID &scriptid, CScript& script) const override;
    promag commented at 10:09 AM on July 20, 2018:

    Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

    nit, CScriptID& scriptid and some more below.

    sipa commented at 6:54 PM on July 20, 2018:

    done

  8. practicalswift commented at 11:21 AM on July 20, 2018: contributor

    Concept ACK

    Thanks for taking another look at this code

  9. promag commented at 1:15 PM on July 20, 2018: member

    Partial utACK. Nice refactors that improve code readability. Some nits regarding code style.

  10. sipa force-pushed on Jul 20, 2018
  11. sipa force-pushed on Jul 20, 2018
  12. sipa force-pushed on Jul 20, 2018
  13. sipa force-pushed on Jul 20, 2018
  14. in src/wallet/rpcwallet.cpp:4464 in d7bb6645ac outdated 
    4460 | @@ -4461,21 +4461,14 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C
4461 |          }
4462 |  
4463 |          SignatureData sigdata;
4464 | -        complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, false), *psbtx.tx, input, sigdata, i, sighash_type);
4465 | +        complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), *psbtx.tx, input, sigdata, i, sighash_type);
    achow101 commented at 6:58 PM on July 20, 2018:

    Shouldn't this change be part of the earlier " Generalize PublicOnlySigningProvider into HidingSigningProvider" commit?

    sipa commented at 7:01 PM on July 20, 2018:

    It could be, but doesn't matter. There is no bip32 information in SignatureData until this commit.

  15. sipa force-pushed on Jul 20, 2018
  16. achow101 commented at 7:00 PM on July 20, 2018: member

    Concept ACK

  17. sipa force-pushed on Jul 20, 2018
  18. in src/script/sign.cpp:625 in cd3394a7b8 outdated 
     619 | @@ -620,7 +620,12 @@ bool PublicOnlySigningProvider::GetCScript(const CScriptID &scriptid, CScript& s
 620 |      return m_provider->GetCScript(scriptid, script);
 621 |  }
 622 |  
 623 | -bool PublicOnlySigningProvider::GetPubKey(const CKeyID &address, CPubKey& pubkey) const
 624 | +bool PublicOnlySigningProvider::GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const
 625 |  {
 626 |      return m_provider->GetPubKey(address, pubkey);
    achow101 commented at 11:11 PM on July 20, 2018:

    In commit "Make SigningProvider expose key origin information"

    You should change address to keyid in this commit instead of "Generalize PublicOnlySigningProvider into HidingSigningProvider"

    sipa commented at 11:40 PM on July 20, 2018:

    Done.

  19. sipa force-pushed on Jul 20, 2018
  20. DrahtBot commented at 1:19 PM on July 22, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #14021 (Import key origin data through importmulti by achow101)
    • #13932 (Additional utility RPCs for PSBT by achow101)
    • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
    • #13671 (Remove the boost/algorithm/string/case_conv.hpp dependency by 251Labs)

    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.

  21. DrahtBot cross-referenced this on Jul 22, 2018 from issue Remove the boost/algorithm/string/case_conv.hpp dependency by l2a5b1
  22. DrahtBot cross-referenced this on Jul 22, 2018 from issue Add CMerkleTx::IsImmatureCoinBase method by Empact
  23. DrahtBot cross-referenced this on Jul 22, 2018 from issue [bugfix] Fix encoding issue for Windows by ken2812221
  24. laanwj added the label RPC/REST/ZMQ on Jul 23, 2018
  25. laanwj commented at 2:05 PM on July 23, 2018: member

    utACK f6ba1ca1e51d044b6fc8398edfa4dd03061572c4 verified that [MOVEONLY] Move ParseHDKeypair is move-only

  26. DrahtBot added the label Needs rebase on Jul 24, 2018
  27. sipa force-pushed on Jul 24, 2018
  28. sipa commented at 10:13 PM on July 24, 2018: member

    Rebased.

  29. DrahtBot removed the label Needs rebase on Jul 24, 2018
  30. DrahtBot cross-referenced this on Jul 25, 2018 from issue [WIP] Full unicode support on Windows by ken2812221
  31. DrahtBot cross-referenced this on Jul 28, 2018 from issue Test for Windows encoding issue by ken2812221
  32. DrahtBot cross-referenced this on Jul 30, 2018 from issue wallet: -avoidreuse with destination filters by kallewoof
  33. DrahtBot cross-referenced this on Aug 1, 2018 from issue Include tinyformat as a subtree by Empact
  34. DrahtBot cross-referenced this on Aug 1, 2018 from issue Inline i64tostr and itostr by Empact
  35. DrahtBot cross-referenced this on Aug 2, 2018 from issue Include tinyformat as a subtree by Empact
  36. DrahtBot cross-referenced this on Aug 2, 2018 from issue Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact
  37. DrahtBot cross-referenced this on Aug 2, 2018 from issue util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by practicalswift
  38. DrahtBot cross-referenced this on Aug 3, 2018 from issue utils: Use _wfopen and _wfreopen on Windows by ken2812221
  39. DrahtBot cross-referenced this on Aug 3, 2018 from issue utils: drop boost::interprocess::file_lock by ken2812221
  40. DrahtBot added the label Needs rebase on Aug 6, 2018
  41. sipa force-pushed on Aug 6, 2018
  42. DrahtBot removed the label Needs rebase on Aug 6, 2018
  43. sipa force-pushed on Aug 7, 2018
  44. sipa commented at 9:43 PM on August 7, 2018: member

    I don't understand why this is causing the no-wallet Travis test to timeout: https://api.travis-ci.org/v3/job/413228232/log.txt

  45. MarcoFalke commented at 9:54 PM on August 7, 2018: member

    Should be fixed by deleting the travis cache and resetting that particular job.

  46. DrahtBot cross-referenced this on Aug 8, 2018 from issue Additional safety checks in PSBT signer by sipa
  47. achow101 commented at 9:15 PM on August 9, 2018: member

    utACK 9c28a0c0ddfb09dcd4d5c0ca83ce5cdae9bc1fa2

  48. DrahtBot cross-referenced this on Aug 9, 2018 from issue Additional utility RPCs for PSBT by achow101
  49. sipa force-pushed on Aug 10, 2018
  50. sipa commented at 3:34 AM on August 10, 2018: member

    Rebased on top of #13917, which has far higher priority.

  51. DrahtBot cross-referenced this on Aug 10, 2018 from issue Always create signatures with Low R values by achow101
  52. DrahtBot added the label Needs rebase on Aug 13, 2018
  53. Only wipe wrong UTXO type data if overwritten by wallet c05712cb59
  54. Additional sanity checks in SignPSBTInput 8254e9950f
  55. Test that a non-witness script as witness utxo is not signed 7c8bffdc24
  56. More tests of signer checks 5df6f089b5
  57. Introduce KeyOriginInfo for fingerprint + path 611ab307fb
  58. Make SigningProvider expose key origin information 84f1f1bfdf
  59. Generalize PublicOnlySigningProvider into HidingSigningProvider 81e1dd5ce1
  60. [MOVEONLY] Move ParseHDKeypath to utilstrencodings 3b01efa0d1
  61. Implement key origin lookup in CWallet 03a99586a3
  62. Pass HD path data through SignatureData cad5dd2368
  63. Make SignPSBTInput operate on a private SignatureData object 917353c8b0
  64. sipa force-pushed on Aug 13, 2018
  65. DrahtBot removed the label Needs rebase on Aug 13, 2018
  66. laanwj added this to the "Blockers" column in a project
  67. in src/wallet/rpcwallet.cpp:4426 in 917353c8b0 
    4463 | -    }
4464 | -    return true;
4465 | -}
4466 | -
4467 | -void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)
4468 | +void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, KeyOriginInfo>& hd_keypaths)
    Empact commented at 10:26 PM on August 20, 2018:

    Looks like this is no longer called.

  68. DrahtBot cross-referenced this on Aug 22, 2018 from issue Import key origin data through descriptors in importmulti by achow101
  69. laanwj commented at 2:22 PM on August 28, 2018: member

    re-utACK 917353c8b0eff4cd95f9a5f7719f6756bb8338b1

  70. laanwj merged this on Aug 28, 2018
  71. laanwj closed this on Aug 28, 2018
  72. laanwj referenced this in commit aa39ca7645 on Aug 28, 2018
  73. MarcoFalke removed this from the "Blockers" column in a project
  74. Bushstar cross-referenced this on Sep 4, 2018 from issue commits from bitcoin/master by Bushstar
  75. meshcollider cross-referenced this on Dec 6, 2018 from issue Require a public key to be retrieved when signing a P2PKH input by achow101
  76. kwvg referenced this in commit e0d83ec619 on Jun 4, 2021
  77. kwvg referenced this in commit f573fb60cb on Jun 4, 2021
  78. kwvg cross-referenced this on Jun 4, 2021 from issue merge #13269, #13425, #13557, #13721, #13666, #13723: BIP 174 PSBT Serializations and RPCs by kwvg
  79. UdjinM6 referenced this in commit e356d8b8d7 on Jun 7, 2021
  80. barrystyle referenced this in commit acb122e6cf on Jun 8, 2021
  81. barrystyle referenced this in commit 9166796714 on Jun 8, 2021
  82. kwvg referenced this in commit 44545672b2 on Jun 9, 2021
  83. kwvg referenced this in commit 9b8652e55b on Jul 9, 2021
  84. kwvg referenced this in commit 158c656bbc on Jul 13, 2021
  85. kwvg referenced this in commit 2a4dee5fa0 on Jul 13, 2021
  86. kwvg referenced this in commit 8b891c2b10 on Jul 13, 2021
  87. PastaPastaPasta referenced this in commit e98241da5d on Jul 13, 2021
  88. bitcoin locked this on Sep 8, 2021
Contributors
sipapromagpracticalswiftachow101DrahtBotlaanwjMarcoFalkeEmpact
Labels
RPC/REST/ZMQ
Linked (view graph)
#14682 RPC getaddressinfo should use CWallet::GetKeyOrigin#14689 Require a public key to be retrieved when signing a P2PKH input
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC