test: various `converttopsbt` check cleanups in rpc_psbt.py #27325

1. 
   theStack commented at 4:56 PM on March 24, 2023: contributor

   In the functional test rpc_psbt.py, some comments around the converttopsbt RPC checks are wrong or outdated and can be removed:

   Error could be either "TX decode failed" (segwit inputs causes parsing to fail) or "Inputs must not have scriptSigs and scriptWitnesses"

   Decoding a valid TX with at least one input always succeeds with the heuristic, i.e. this comment is not right and we can assert for the error string "Inputs must not have scriptSigs and scriptWitnesses" on the calls below.

   We must set iswitness=True because the serialized transaction has inputs and is therefore a witness transaction

   This is also unneeded (and confusing, w.r.t. "is therefore a witness transaction"?), for a TX with one input there is no need to set the iswitness parameter. For sake of completeness, we still keep one variant where iswitness is explicitly set to true.

   Lastly, there is a superflous converttopsbt call on the raw tx which is the same as just about ~10 lines above, so it can be removed.

2. test: various `converttopsbt` check cleanups in rpc_psbt.py

   In the functional test rpc_psbt.py, some comments around the
   `converttopsbt` RPC checks are wrong or outdated and can be
   removed:
   
   > Error could be either "TX decode failed" (segwit inputs causes
   > parsing to fail) or "Inputs must not have scriptSigs and
   > scriptWitnesses"
   
   Decoding a valid TX with at least one input always succeeds with the
   heuristic, i.e. this comment is not right and we can assert for the
   error string "Inputs must not have scriptSigs and scriptWitnesses"
   on the calls below.
   
   > We must set iswitness=True because the serialized transaction has
   > inputs and is therefore a witness transaction
   
   This is also unneeded (and confusing, w.r.t. "is therefore a witness
   transaction"?), for a TX with one input there is no need to set the
   `iswitness` parameter. For sake of completeness, we still keep one
   variant where iswitness is explicitly set to true.
   
   Lastly, there is a superflous `converttopsbt` call on the raw tx which
   is the same as just about ~10 lines above, so it can be removed.

   afc2dd5484
3. 
   DrahtBot commented at 4:56 PM on March 24, 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	ismaelsadeeq, achow101

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

4. DrahtBot added the label Tests on Mar 24, 2023
5. fanquake requested review from instagibbs on Mar 24, 2023
6. 
   ismaelsadeeq commented at 4:04 PM on April 3, 2023: member

   tested ACK afc2dd54848fa70ed408ae259420ff8648f21efc

   I ran the test and it passed.

7. fanquake assigned achow101 on May 4, 2023
8. 
   achow101 commented at 3:01 PM on May 4, 2023: member

   ACK afc2dd54848fa70ed408ae259420ff8648f21efc

9. achow101 merged this on May 4, 2023
10. achow101 closed this on May 4, 2023

    ---

11. theStack deleted the branch on May 4, 2023
12. sidhujag referenced this in commit 1755f61fac on May 5, 2023
13. bitcoin locked this on May 3, 2024

Review Requested
instagibbs