test, rpc: invalid sighashtype coverage #28166

1. 
   jonatack commented at 11:40 PM on July 26, 2023: member

   Add test coverage for passing an invalid sighashtype to RPCs signrawtransactionwithwallet, signrawtransactionwithkey, walletprocesspsbt, and descriptorprocesspsbt.

2. 
   DrahtBot commented at 11:40 PM on July 26, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	MarcoFalke, brunoerg

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. jonatack force-pushed on Jul 27, 2023
4. in test/functional/rpc_psbt.py:330 in 9dde725f45 outdated

    326 | @@ -327,7 +327,7 @@ def run_test(self):
    327 |                  assert_raises_rpc_error(-3, "Invalid amount",
    328 |                      self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: invalid_value, "add_inputs": True})
    329 |          # Test fee_rate values that cannot be represented in sat/vB.
    330 | -        for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]:
    331 | +        for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999]:
   

   ---

   ---

   

   maflcko commented at 5:53 AM on July 27, 2023:

   in 9dde725f45c227189335d8745955c688aacd99c4: I think it is fine if you rephrase the commit body in your own words. The comment you are referring to (and not link to) may very well be wrong.

   ---

   

   jonatack commented at 1:03 PM on July 27, 2023:

   Done

   ---

   

   maflcko commented at 1:16 PM on July 27, 2023:

   unrelated: Just for reference, 31.99999999 is really 31.99999998999999917259629000909626483917236328125

5. maflcko approved
6. 
   maflcko commented at 5:57 AM on July 27, 2023: member

   nit: I think it is fine to squash all the sighhashtype commits into one. They all do the same thing, and splitting it just makes review work harder, because at a minimum for each commit there is additional overhead for reviewers to check that a commit doesn't add a backdoor and removes it in the next commit.

7. test: add coverage for passing an invalid sighashtype

   in RPCs descriptorprocesspbst, walletprocesspbst, signrawtransactionwithkey,
   and signrawtransactionwithwallet.

   647d95aae9
8. test: use common assert_signing_completed_successfully helper c3f203387d
9. test: remove redundant test values

   as they are parsed identically.
   
   See AmountFromValue() / ParseFixedPoint() / UniValue#getValStr()

   90c8f79e94
10. jonatack force-pushed on Jul 27, 2023
11. 
    jonatack commented at 1:05 PM on July 27, 2023: member

    Thanks @MarcoFalke, done.

12. 
    maflcko commented at 4:39 PM on July 27, 2023: member

    lgtm ACK 90c8f79e945863f3818748b86572948d1558aec3 🎥

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: lgtm ACK 90c8f79e945863f3818748b86572948d1558aec3 🎥
    4nQVxrCzq3s4J1Z4BUrM5/b3iTRnVbJgJm6YkPfqhKZXEU7aCffkfhrz97XrUTitu/61d3dFQ202n8xosxSfBQ==
    

    </details>

13. 
    brunoerg commented at 10:09 PM on July 27, 2023: contributor

    light crACK 90c8f79e945863f3818748b86572948d1558aec3

14. fanquake requested review from achow101 on Jul 28, 2023
15. fanquake merged this on Aug 1, 2023
16. fanquake closed this on Aug 1, 2023

    ---

17. jonatack deleted the branch on Aug 1, 2023
18. sidhujag referenced this in commit e16fcb75b3 on Aug 9, 2023
19. bitcoin locked this on Jul 31, 2024

Contributors

jonatackDrahtBotmaflckobrunoerg

Review Requested
achow101