test: Add combinerawtransaction test to rpc_createmultisig #31249

1. 
   achow101 commented at 6:35 pm on November 7, 2024: member

   The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.

   Split from #28710

2. 
   DrahtBot commented at 6:35 pm on November 7, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31249.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, BrandonOdiwuor, Abdulkbk, brunoerg, rkrux

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31250 (wallet: Disable creating and loading legacy wallets by achow101)

   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.

3. DrahtBot added the label Tests on Nov 7, 2024
4. 
   maflcko commented at 10:09 am on November 8, 2024: member

   Forgot to move ab98e6fd039 as well? Otherwise

   lgtm ACK af4d23178b420f46196fbace2176ce1fe94ed9cd

5. BrandonOdiwuor approved
6. 
   BrandonOdiwuor commented at 1:14 pm on November 8, 2024: contributor

   Code Review ACK af4d23178b420f46196fbace2176ce1fe94ed9cd
7. test: Add combinerawtransaction test to rpc_createmultisig

   The only coverage of combinerawtransaction is in a legacy wallet only
   test. So also use it in rpc_createmultisig so that this RPC remains
   tested after the legacy wallet is removed.

   83fab3212c
8. achow101 force-pushed on Nov 8, 2024
9. 
   achow101 commented at 4:50 pm on November 8, 2024: member

   Forgot to move ab98e6f as well?

   Pulled those in too

10. 
    maflcko commented at 5:02 pm on November 8, 2024: member

    re-ACK 83fab3212c91d91fc5502f940c901a07772ff747
11. DrahtBot requested review from BrandonOdiwuor on Nov 8, 2024
12. 
    BrandonOdiwuor commented at 6:21 pm on November 8, 2024: contributor

    Re-ACK 83fab3212c91d91fc5502f940c901a07772ff747
13. maflcko added this to the milestone 29.0 on Nov 9, 2024
14. 
    Abdulkbk commented at 11:12 am on November 11, 2024: none

    ACK 83fab3212c91d91fc5502f940c901a07772ff747
15. brunoerg approved
16. 
    brunoerg commented at 9:01 pm on November 11, 2024: contributor

    code review ACK 83fab3212c91d91fc5502f940c901a07772ff747
17. in test/functional/rpc_createmultisig.py:195 in 83fab3212c

    193@@ -194,13 +194,19 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi):
    194         assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
    195 
    

    ---

    ---

    

    rkrux commented at 7:32 am on November 12, 2024:

    Lacking before this change as well, can we add a comment here just like there is one in all the code blocks above and below?

    sign the rawtx with nsigs-1 signatures, then sign with the last sig, and then combine to create fully signed tx

18. in test/functional/rpc_createmultisig.py:198 in 83fab3212c

    193@@ -194,13 +194,19 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi):
    194         assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
    195 
    196         rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs)
    197-        rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs)
    198-        assert rawtx3['complete']
    199-
    200-        tx = node0.sendrawtransaction(rawtx3["hex"], 0)
    201+        assert_equal(rawtx2["complete"], False)
    202+        rawtx3 = node2.signrawtransactionwithkey(rawtx, [priv_keys[-1]], prevtxs)
    

    ---

    ---

    

    rkrux commented at 7:35 am on November 12, 2024:

    Although not a new change and not incorrect as well, but I noted that the last signature is from the priv_keys[-1], and not priv_keys[nsigs]. Maybe the author intended to test signatures from non-consecutive keys as well.
19. in test/functional/rpc_createmultisig.py:202 in 83fab3212c

    201+        assert_equal(rawtx2["complete"], False)
    202+        rawtx3 = node2.signrawtransactionwithkey(rawtx, [priv_keys[-1]], prevtxs)
    203+        assert_equal(rawtx3["complete"], False)
    204+        assert_raises_rpc_error(-22, "TX decode failed", node2.combinerawtransaction, [rawtx2['hex'], rawtx3['hex'] + "00"])
    205+        assert_raises_rpc_error(-22, "Missing transactions", node2.combinerawtransaction, [])
    206+        combined_rawtx = node2.combinerawtransaction([rawtx2["hex"], rawtx3["hex"]])
    

    ---

    ---

    

    rkrux commented at 7:38 am on November 12, 2024:

    For readability

    0        full_signed_tx = node2.combinerawtransaction([rawtx2["hex"], rawtx3["hex"]])
    

20. rkrux approved
21. 
    rkrux commented at 7:44 am on November 12, 2024: none

    tACK 83fab3212c91d91fc5502f940c901a07772ff747

    Make and functional tests successful.

    Thanks for ensuring coverage for this RPC is not lost.

22. fanquake merged this on Nov 12, 2024
23. fanquake closed this on Nov 12, 2024

    ---

Contributors
achow101 DrahtBot maflcko BrandonOdiwuor Abdulkbk brunoerg rkrux

Labels
Tests

Milestone
29.0