wallet: AvailableCoins, simplify output script type acquisition #25933

1. 
   furszy commented at 8:51 PM on August 25, 2022: member

   There is an unnecessary ExtractDestination() call and subsequent result parse into an CScriptID.

   The Solver() call, which we are already doing below anyway, retrieves the script type and, in the P2SH case, the program id.

2. DrahtBot added the label Wallet on Aug 25, 2022
3. 
   theStack commented at 11:54 PM on August 25, 2022: contributor

   Concept ACK

4. 
   DrahtBot commented at 5:02 AM on August 26, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25183 (rpc: Filter inputs by type during CoinSelection by aureleoules)

   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.

5. 
   aureleoules commented at 9:23 AM on September 13, 2022: member

   ACK 24c82ee0987a39a551f9ab40b95b72e3ed1e224f - LGTM

6. in src/wallet/spend.cpp:272 in 24c82ee098 outdated

    275 | -                if (!ExtractDestination(output.scriptPubKey, destination))
    276 | -                    continue;
    277 | -                const CScriptID& hash = CScriptID(std::get<ScriptHash>(destination));
    278 | -                if (!provider->GetCScript(hash, script))
    279 | -                    continue;
    280 | +            if (type == TxoutType::SCRIPTHASH && solvable) {
   

   ---

   ---

   

   rajarshimaitra commented at 11:10 AM on September 17, 2022:

   In the comment above it is mentioned that we check this if the output is "spendable". But in the conditional we are checking for "solvable", which I think is enough to serve the purpose. Should the comment be updated in that light?

   ---

   

   furszy commented at 1:27 PM on September 17, 2022:

   yeah. The comment isn't accurate. Solvable refers to the wallet knowing how to create the unlocking script that spends the output (which is what we are looking for here), while spendable refers to whether the wallet has the key/s to sign the input or not.

7. 
   rajarshimaitra commented at 11:29 AM on September 17, 2022: contributor

   tACK 24c82ee0987a39a551f9ab40b95b72e3ed1e224f

   Just one non-blocking nit comment not related to changes of this PR.

8. wallet: AvailableCoins, simplify output script type acquisition 58b7df3caa
9. furszy force-pushed on Sep 17, 2022
10. 
    furszy commented at 1:33 PM on September 17, 2022: member

    Done, added @rajarshimaitra feedback.

11. aureleoules approved
12. 
    aureleoules commented at 1:56 PM on September 20, 2022: member

    re-ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30

13. rajarshimaitra approved
14. 
    rajarshimaitra commented at 4:42 AM on September 21, 2022: contributor

    ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30

15. maflcko requested review from achow101 on Sep 21, 2022
16. w0xlt approved
17. 
    w0xlt commented at 12:22 PM on September 21, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/25933/commits/58b7df3caa21519de61e10f6ee42f0be9ac3cc30

18. 
    achow101 commented at 3:23 PM on September 21, 2022: member

    ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30

19. achow101 merged this on Sep 21, 2022
20. achow101 closed this on Sep 21, 2022

    ---

21. sidhujag referenced this in commit e08bc980a7 on Sep 23, 2022
22. furszy deleted the branch on May 27, 2023
23. bitcoin locked this on May 26, 2024

Contributors

furszytheStackDrahtBotaureleoulesrajarshimaitraw0xltachow101

Review Requested
achow101