Add iswitness parameter to decode- and fundrawtransaction RPCs #11178
Conversation
meshcollider
changed the title
meshcollider
changed the title
|
@achow101 @roconnor-blockstream you might want to take a look at this :) |
src/core_read.cpp
Outdated
| @@ -108,7 +108,7 @@ bool CheckTxScriptsSanity(const CMutableTransaction& tx) | |||
| return true; | |||
| } | |||
|
|
|||
| bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx, bool fTryNoWitness) | |||
| bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx, bool fTryNoWitness, bool fTryWitness) | |||
|
Changed style as requested, thanks @achow101 |
|
Please don't change the style of existing code, only the new code... :/ |
|
@luke-jr the style refactor only touches a tiny amount of code inside that single function, in a similar way to adding braces to existing if-statements when modifying nearby code. I'd argue that this small change to existing code style is worth it for the consistency, especially between try_witness and try_no_witness |
src/core_read.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
|
|
||
| return true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea yep, done
src/rpc/rawtransaction.cpp
Outdated
| "\nReturn a JSON object representing the serialized, hex-encoded transaction.\n" | ||
|
|
||
| "\nArguments:\n" | ||
| "1. \"hexstring\" (string, required) The transaction hex string\n" | ||
| "2. iswitness (boolean, optional) Whether the transaction hex is a serialized witness transaction \n" |
src/core_read.cpp
Outdated
| return false; | ||
| } | ||
| } | ||
| catch (const std::exception&) { |
src/core_read.cpp
Outdated
| try { | ||
| ssData >> tx; | ||
| if (!ssData.empty()) { | ||
| return false; |
There was a problem hiding this comment.
Even shorter: invert this test and make it return true.
src/core_read.cpp
Outdated
| if (ssData.empty()) { | ||
| return true; | ||
| } | ||
| } catch (const std::exception&) { | ||
| return false; |
There was a problem hiding this comment.
And now this line is unnecessary as well :)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
src/core_read.cpp
Outdated
| CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS); | ||
| try { | ||
| ssData >> tx; | ||
| if (ssData.eof() && CheckTxScriptsSanity(tx)) { | ||
| if (ssData.eof() && (CheckTxScriptsSanity(tx) || !try_witness)) { |
There was a problem hiding this comment.
One final tiny nit: swap the arguments, as checking a boolean is much faster than CheckTxScriptsSanity.
|
utACK 3d16658226cd08911200dbd90db762dc3409e443 |
|
Rebased |
|
utACK 6f39ac0 |
…on RPCs 6f39ac0 Add test for decoderawtransaction bool (MeshCollider) bbdbe80 Add iswitness parameter to decode- and fundrawtransaction RPCs (MeshCollider) Pull request description: Suggested in #10481 (comment), this adds the option to explicitly choose whether a serialized transaction should be decoded as a witness or non-witness transaction rather than relying on the heuristic checks in #10481. The parameter defaults to relying on #10481 if not included, but it overrides that if included. Tree-SHA512: d4846a5bb7d64dc19c516445488b00af329fc1f4181d9dfdf9f2382a086568edc98250a4ac7594e24a1bc231dfdee53c699b12c8380c355b920a67cc6770b7a9
Looks like another `assert_raises_jsonrpc` snuck in with bitcoin#11178. Change it to `assert_raises_rpc_error`.
4508519 test: Fix rawtransactions test (Wladimir J. van der Laan) Pull request description: Looks like another `assert_raises_jsonrpc` snuck in with #11178. Change it to `assert_raises_rpc_error`. Tree-SHA512: c2c2fb78be5dcc490981896cf60be1fba0b41c385a4f18de084b2e0a042c5b06bf6da617d3bf3b7e585b728554f5c8e1814b85045ba542cca9dfb7b826fda75a
Looks like another `assert_raises_jsonrpc` snuck in with bitcoin#11178. Change it to `assert_raises_rpc_error`.
Looks like another `assert_raises_jsonrpc` snuck in with bitcoin#11178. Change it to `assert_raises_rpc_error`.
Suggested in #10481 (comment), this adds the option to explicitly choose whether a serialized transaction should be decoded as a witness or non-witness transaction rather than relying on the heuristic checks in #10481. The parameter defaults to relying on #10481 if not included, but it overrides that if included.