Add iswitness parameter to decode- and fundrawtransaction RPCs #11178

 107 | @@ -108,7 +108,7 @@ bool CheckTxScriptsSanity(const CMutableTransaction& tx)
 108 |      return true;
 109 |  }
 110 |  
 111 | -bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx, bool fTryNoWitness)
 112 | +bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx, bool fTryNoWitness, bool fTryWitness)

achow101 commented at 3:35 PM on August 30, 2017:

snake_case

 157 | -    catch (const std::exception&) {
 158 | -        return false;
 159 | -    }
 160 | -
 161 | +    
 162 |      return true;

sipa commented at 8:37 PM on September 3, 2017:

If you move this return true; inside the if (try_witness) block, the if(!try_witness) inside the catch above becomes unnecessary. I think that'd be a cleaner flow.

meshcollider commented at 10:33 PM on September 3, 2017:

Good idea yep, done

 425 | +            "decoderawtransaction \"hexstring\" ( iswitness )\n"
 426 |              "\nReturn a JSON object representing the serialized, hex-encoded transaction.\n"
 427 |  
 428 |              "\nArguments:\n"
 429 |              "1. \"hexstring\"      (string, required) The transaction hex string\n"
 430 | +            "2. iswitness          (boolean, optional) Whether the transaction hex is a serialized witness transaction \n"

sipa commented at 8:39 PM on September 3, 2017:

Nit: trailing space

meshcollider commented at 10:33 PM on September 3, 2017:

Fixed

 139 | +            ssData >> tx;
 140 | +            if (!ssData.empty()) {
 141 | +                return false;
 142 | +            }
 143 | +        }
 144 | +        catch (const std::exception&) {

sipa commented at 10:35 PM on September 3, 2017:

Nit: catch on the same line as }.

 136 | +    if (try_witness) {
 137 | +        CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION);
 138 | +        try {
 139 | +            ssData >> tx;
 140 | +            if (!ssData.empty()) {
 141 | +                return false;

sipa commented at 10:35 PM on September 3, 2017:

Even shorter: invert this test and make it return true.

meshcollider commented at 10:45 PM on September 3, 2017:

Nice :+1:

 145 | +            ssData >> tx;
 146 | +            if (ssData.empty()) {
 147 | +                return true;
 148 | +            }
 149 | +        } catch (const std::exception&) {
 150 |              return false;

sipa commented at 10:50 PM on September 3, 2017:

And now this line is unnecessary as well :)

meshcollider commented at 10:54 PM on September 3, 2017:

Heh, changed. Next you'll want me to move the try-catch outside the if statements so there only needs to be one of them ;) Thanks for reviewing!

sipa commented at 11:06 PM on September 3, 2017:

Nope, that wouldn't actually work. If both flags were true, and the first attempt to deserialize throws, you still want to try the second.

 123 | +    if (try_no_witness) {
 124 |          CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS);
 125 |          try {
 126 |              ssData >> tx;
 127 | -            if (ssData.eof() && CheckTxScriptsSanity(tx)) {
 128 | +            if (ssData.eof() && (CheckTxScriptsSanity(tx) || !try_witness)) {

sipa commented at 11:58 PM on September 3, 2017:

One final tiny nit: swap the arguments, as checking a boolean is much faster than CheckTxScriptsSanity.