signrawtransactionwithkey: report error when missing redeemScript/witnessScript #16250

1. 
   ajtowns commented at 11:45 am on June 20, 2019: member

   Adding support for “witnessScript” as an alternative to “redeemScript” when using “signrawtransactionwithkey” meant that the RPCTypeCheckObj() call in SignTransaction can’t error out just because either parameter is missing – it’s only a problem if both are missing, which isn’t a state RPCTypeCheckObj() tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:

   error code: -8
   error message:
   Missing redeemScript/witnessScript
   

   Fixes: #16249

2. DrahtBot added the label RPC/REST/ZMQ on Jun 20, 2019
3. DrahtBot added the label Tests on Jun 20, 2019
4. in src/rpc/rawtransaction_util.cpp:225 in 50436e5975 outdated

   220@@ -221,6 +221,9 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
   221                     // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
   222                     keystore->AddCScript(GetScriptForWitness(witnessScript));
   223                 }
   224+                if (rs.isNull() && ws.isNull()) {
   225+                    throw JSONRPCError(RPC_TYPE_ERROR, "Missing redeemScript/witnessScript");
   

   ---

   ---

   

   promag commented at 3:19 pm on June 21, 2019:

   How about:

   0    RPC_INVALID_PARAMETER           = -8,  //!< Invalid, missing or duplicate parameter
   

5. 
   promag commented at 3:20 pm on June 21, 2019: member

   I think RPCTypeCheckObj(...) in L202 could be removed?

   Concept ACK.

6. 
   meshcollider commented at 8:09 am on June 24, 2019: contributor

   Looks good, agree with RPC_INVALID_PARAMETER though
7. fanquake added the label Waiting for author on Jun 24, 2019
8. signrawtransactionwithkey: report error when missing redeemScript/witnessScript param 01174596e6
9. ajtowns force-pushed on Jun 25, 2019
10. 
    ajtowns commented at 2:58 am on June 25, 2019: member

    I think RPCTypeCheckObj(...) in L202 could be removed?

    Could be, but I think it gives a cleaner error for bad params, eg "redeemScript": 5 currently gives:

    Expected type string for redeemScript, got number
    

    vs leaving the error handling up to ParseHexV which gives:

    redeemScript must be hexadecimal string (not '')
    

    Updated to use RPC_INVALID_PARAMETER.

11. fanquake removed the label Waiting for author on Jun 25, 2019
12. 
    meshcollider commented at 2:56 am on June 26, 2019: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/16250/commits/01174596e69568c434198a86f54cb9ea6740e6c2
13. 
    promag commented at 9:13 pm on July 1, 2019: member

    ACK 01174596e. Could also write test without dict/del:

     0         outval = value - decimal.Decimal("0.00001000")
     1         rawtx = node2.createrawtransaction([{"txid": txid, "vout": vout}], [{self.final: outval}])
     2
     3-        prevtx_err = dict(prevtxs[0])
     4-        del prevtx_err["redeemScript"]
     5-
     6-        assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
     7+        assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [{k:v for k,v in prevtxs[0].items() if k != "redeemScript"}])
     8
     9         rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs)
    10         rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs)
    

14. MarcoFalke referenced this in commit e06067387e on Jul 2, 2019
15. MarcoFalke merged this on Jul 2, 2019
16. MarcoFalke closed this on Jul 2, 2019

    ---

17. sidhujag referenced this in commit 0c8c2ad2de on Jul 3, 2019
18. ajtowns referenced this in commit 1944475822 on Jul 3, 2019
19. ajtowns referenced this in commit 1dc357dabb on Jul 5, 2019
20. fanquake referenced this in commit 1fb747a800 on Jul 7, 2019
21. HashUnlimited referenced this in commit e60161f981 on Aug 23, 2019
22. Bushstar referenced this in commit 8a4968afce on Aug 24, 2019
23. Bushstar referenced this in commit b8fc5c4cd3 on Aug 24, 2019
24. Bushstar referenced this in commit a81c9030b7 on Aug 24, 2019
25. Bushstar referenced this in commit cac4ddb16f on Aug 24, 2019
26. fanquake referenced this in commit 2296fe65f5 on Sep 11, 2019
27. DrahtBot locked this on Dec 16, 2021

Contributors
ajtowns promag meshcollider

Labels
Tests RPC/REST/ZMQ