rpc: Document iswitness flag and fix bug in converttopsbt #15899

1. 
   MarcoFalke commented at 2:17 PM on April 26, 2019: member

   When a serialized transaction has inputs, there is no risk in only trying to deserialize it with witness allowed. (This is how all transactions from p2p are deserialized.) In fact, it would avoid a common issue where a transaction with inputs can be deserialized in two ways:

   • Fixes #12989
   • Fixes #15872
   • Fixes #15701
   • Fixes #13738
   • ...

   When a serialized transaction has no inputs, there is no risk in only trying to deserialze it with witness disallowed. (A transaction without inputs can't have corresponding witness data)

2. fanquake added the label RPC/REST/ZMQ on Apr 26, 2019
3. MarcoFalke added the label Needs backport on Apr 26, 2019
4. MarcoFalke added this to the milestone 0.18.1 on Apr 26, 2019
5. MarcoFalke force-pushed on Apr 26, 2019
6. MarcoFalke force-pushed on Apr 26, 2019
7. 
   MarcoFalke commented at 10:35 PM on May 13, 2019: member

   review beg :pray: @meshcollider As the author of the docstring, you seem qualified to review the doc change? @achow101 As the author of converttopsbt, you seem qualified to review the code change?

8. 
   achow101 commented at 10:48 PM on May 13, 2019: member

   utACK fa8630d268dc65e097ead9962d9d8af2301182ca

9. in src/rpc/rawtransaction.cpp:443 in faee652f97 outdated

    436 | @@ -437,8 +437,10 @@ static UniValue decoderawtransaction(const JSONRPCRequest& request)
    437 |                  "\nReturn a JSON object representing the serialized, hex-encoded transaction.\n",
    438 |                  {
    439 |                      {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction hex string"},
    440 | -                    {"iswitness", RPCArg::Type::BOOL, /* default */ "depends on heuristic tests", "Whether the transaction hex is a serialized witness transaction\n"
    441 | -            "                         If iswitness is not present, heuristic tests will be used in decoding"},
    442 | +                    {"iswitness", RPCArg::Type::BOOL, /* default */ "depends on heuristic tests",
    443 | +                        "Whether the transaction hex is a serialized witness transaction.\n"
    444 | +                        "This boolean should reflect whether the transaction has inputs, if known by the caller.\n"
    445 | +                        "If it is not set, heuristic tests will be used in decoding."},
   

   ---

   ---

   

   ryanofsky commented at 3:51 PM on May 16, 2019:

   In commit "rpc: Document iswitness flag" (faee652f971e5422c4df1dff3f36c9089aaf587f)

   Maybe drop this sentence and just use the same "If not present, If true..., If false..." text from converttopsbt. I think the converttopsbt explanation is a little clearer, and that it's better to keep the text consistent in all three methods so readers don't have to parse text closely to see where differences are.

   Sipa's explanation and advice from the bug also seem very helpful and so might be worth incorporating too:

   • #12989 (comment)
   • #12989 (comment)

   ---

   

   MarcoFalke commented at 7:45 PM on May 16, 2019:

   Force pushed to use the same docstring everywhere

10. ryanofsky approved
11. 
    ryanofsky commented at 4:19 PM on May 16, 2019: member

    utACK fa8630d268dc65e097ead9962d9d8af2301182ca

12. rpc: Switch touched RPCs to IsValidNumArgs fa5c5cd141
13. MarcoFalke force-pushed on May 16, 2019
14. MarcoFalke force-pushed on May 16, 2019
15. rpc: bugfix: Properly use iswitness in converttopsbt

    Also explain the param in all RPCs

    fa499b5f02
16. MarcoFalke force-pushed on May 16, 2019
17. ryanofsky approved
18. 
    ryanofsky commented at 9:13 PM on May 16, 2019: member

    utACK fa499b5f027f77c0bf13699852c8c06f78e27bef. Changes since last review: consolidating commits and making iswitness documentation the same across methods.

19. PastaPastaPasta approved
20. 
    PastaPastaPasta commented at 2:39 AM on June 6, 2019: contributor

    utACK fa499b5f027f77c0bf13699852c8c06f78e27bef

21. 
    MarcoFalke commented at 3:12 PM on June 6, 2019: member

    Anything left to do here?

22. 
    meshcollider commented at 12:48 PM on June 18, 2019: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/15899/commits/fa499b5f027f77c0bf13699852c8c06f78e27bef

23. meshcollider merged this on Jun 18, 2019
24. meshcollider closed this on Jun 18, 2019

    ---

25. meshcollider referenced this in commit 22b6c4ed75 on Jun 18, 2019
26. MarcoFalke deleted the branch on Jun 18, 2019
27. MarcoFalke referenced this in commit bb36ac82ef on Jun 18, 2019
28. MarcoFalke referenced this in commit 0023c97890 on Jun 18, 2019
29. MarcoFalke removed the label Needs backport on Jun 18, 2019
30. sidhujag referenced this in commit 6832c16b75 on Jun 19, 2019
31. HashUnlimited referenced this in commit 770abf6de8 on Aug 23, 2019
32. HashUnlimited referenced this in commit 24d8b87fa7 on Aug 23, 2019
33. Bushstar referenced this in commit df3238a580 on Aug 24, 2019
34. Bushstar referenced this in commit 2a063e80af on Aug 24, 2019
35. vijaydasmp referenced this in commit 090d778a0c on Dec 10, 2021
36. vijaydasmp referenced this in commit 0b8a40a785 on Dec 10, 2021
37. vijaydasmp referenced this in commit 5177a86893 on Dec 11, 2021
38. vijaydasmp referenced this in commit 6e1eadf146 on Dec 11, 2021
39. vijaydasmp referenced this in commit d0de12e763 on Dec 11, 2021
40. vijaydasmp referenced this in commit 4304d6ae25 on Dec 11, 2021
41. vijaydasmp referenced this in commit 17e2687ee6 on Dec 13, 2021
42. vijaydasmp referenced this in commit 20d674f397 on Dec 14, 2021
43. vijaydasmp referenced this in commit 8a20aa7f42 on Dec 14, 2021
44. DrahtBot locked this on Feb 15, 2022

Contributors

MarcoFalkeachow101ryanofskyPastaPastaPastameshcollider

Milestone
0.18.1