Add option to return non-segwit serialization via rpc
Libraries may not be upgraded in time to handle the new serialization when segwit blocks and transactions come into the mainchain. This flag would allow people to use the RPC interface uninterrupted and intentionally upgrade as needed.
It’s affecting users today.
update: I have added a command line flag instead. See below.
I don’t see a use case to exclude only segwit signatures. Instead, it should strip all signatures, whether segwit or otherwise…
(Also note this cannot affect users today, since segwit is not active today…)
scriptSigs are no more or less useful to strip than segwit signatures.
Of course they are, because they are included in the txid calculation.
I understand there’s a user who is complaining, but I thought our view on this issue was that people just shouldn’t upgrade their software until their rpc clients, zmq clients, REST clients etc were ready for segwit serialization. I haven’t looked but I assume there are a lot of places in the code where this issue (of not knowing whether the consumer wants witnesses or not) would come up?
I suppose we could try to do something like what @sipa suggests: a command line flag indicating the serialization to use (with/without witness) for all downstream consumers; it just seems tedious to get right. Might it not be better to just add a python script to contrib that takes a witness-serialized tx and returns a serialization without witness? I guess that would still require downstream library changes, so maybe that doesn’t help.
I agree that adding another boolean for legacy purposes to the rpc api was suboptimal, so I have added a command line argument at startup to offer the same functionality called -rpcserialversion with a default value of 1. If set to 0 the NO_WITNESS flag is included in the same places as before. I believe this covers the major use-cases, which is passing along serializations of transactions understood by legacy libraries and the like(which is the activity the p2p layer takes great care to abide by).
I can squash if this is deemed superior.
115@@ -116,9 +116,9 @@ string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDecode)
116 return str;
117 }
118
119-string EncodeHexTx(const CTransaction& tx)
120+string EncodeHexTx(const CTransaction& tx, const bool fNonWitnessTx)
227@@ -228,7 +228,7 @@ static bool rest_block(HTTPRequest* req,
228 return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
229 }
230
231- CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION);
232+ CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | (GetArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0) ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
3558@@ -3559,6 +3559,14 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
3559 return commitment;
3560 }
3561
3562+int RPCSerializationFlags()
main.cpp to server.cpp or core_write.cpp.
696@@ -697,6 +697,7 @@ UniValue getblock(const JSONRPCRequest& request)
697 "\nArguments:\n"
698 "1. \"hash\" (string, required) The block hash\n"
699 "2. verbose (boolean, optional, default=true) true for a json object, false for the hex encoded data\n"
700+ "3. nonwit (boolean, optional, default=false) whether to return a non-witness block in non-verbose mode\n"
218+ print("Verify block and transaction serialization rpcs return differing serializations depending on rpc serialization flag")
219+ # Note: node1 has version 2, which is simply >0 and will catch future upgrades in tests
220+ assert(self.nodes[2].getblock(block[0], False) != self.nodes[0].getblock(block[0], False))
221+ assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
222+ for i in range(len(segwit_tx_list)):
223+ #Coinbase can not be a segwit transcation
219+ # Note: node1 has version 2, which is simply >0 and will catch future upgrades in tests
220+ assert(self.nodes[2].getblock(block[0], False) != self.nodes[0].getblock(block[0], False))
221+ assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
222+ for i in range(len(segwit_tx_list)):
223+ #Coinbase can not be a segwit transcation
224+ if "coinbase" in segwit_tx_list[i]:
segwit_tx_list a list of txids?
24@@ -25,7 +25,7 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN
25
26 // core_write.cpp
27 std::string FormatScript(const CScript& script);
28-std::string EncodeHexTx(const CTransaction& tx);
29+std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
Cribbing from IRC for historical reasons @petertodd :
re: luke-jr’s point on “stripped sigs”, lots of code gets written that calculates its own txids and isn’t using the RPC for that, e.g. python-bitcoinlib RPC code would likely do that, so stripped sigs mode isn’t useful there; 100% backwards compatibility is
221+ assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
222+ for i in range(len(segwit_tx_list)):
223+ assert(self.nodes[2].getrawtransaction(segwit_tx_list[i]) != self.nodes[0].getrawtransaction(segwit_tx_list[i]))
224+ assert(self.nodes[1].getrawtransaction(segwit_tx_list[i], 0) == self.nodes[2].getrawtransaction(segwit_tx_list[i]))
225+ assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) != self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
226+ assert(self.nodes[1].getrawtransaction(segwit_tx_list[i]) == self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
rpcserializationversion=1 handling were busted.)
FYI this did the trick for me:
0diff --git a/qa/rpc-tests/segwit.py b/qa/rpc-tests/segwit.py
1index a618aec..e8a5512 100755
2--- a/qa/rpc-tests/segwit.py
3+++ b/qa/rpc-tests/segwit.py
4@@ -219,10 +219,13 @@ class SegWitTest(BitcoinTestFramework):
5 assert(self.nodes[2].getblock(block[0], False) != self.nodes[0].getblock(block[0], False))
6 assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
7 for i in range(len(segwit_tx_list)):
8+ from test_framework.mininode import FromHex
9+ tx = FromHex(CTransaction(), self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
10 assert(self.nodes[2].getrawtransaction(segwit_tx_list[i]) != self.nodes[0].getrawtransaction(segwit_tx_list[i]))
11 assert(self.nodes[1].getrawtransaction(segwit_tx_list[i], 0) == self.nodes[2].getrawtransaction(segwit_tx_list[i]))
12 assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) != self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
13 assert(self.nodes[1].getrawtransaction(segwit_tx_list[i]) == self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
14+ assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) == bytes_to_hex_str(tx.serialize_without_witness()))
15
16 print("Verify witness txs without witness data are invalid after the fork")
17 self.fail_mine(self.nodes[2], wit_ids[NODE_2][WIT_V0][2], False)
“no objection” utACK ebe6c30a2763a281425de35fd7b2f5dc87f48199
It’s good that the RPC handling is tested with the segwit.py test; it’d be nice to update the zmq and rest python tests as well, but I think that can be done later.
379@@ -380,6 +380,7 @@ std::string HelpMessage(HelpMessageMode mode)
380 strUsage += HelpMessageOpt("-port=<port>", strprintf(_("Listen for connections on <port> (default: %u or testnet: %u)"), Params(CBaseChainParams::MAIN).GetDefaultPort(), Params(CBaseChainParams::TESTNET).GetDefaultPort()));
381 strUsage += HelpMessageOpt("-proxy=<ip:port>", _("Connect through SOCKS5 proxy"));
382 strUsage += HelpMessageOpt("-proxyrandomize", strprintf(_("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)"), DEFAULT_PROXYRANDOMIZE));
383+ strUsage += HelpMessageOpt("-rpcserialversion", strprintf(_("Sets the serialization of raw transaction or block hex returned in non-verbose mode, non-segwit(0) or segwit(>0) (default: %d)"), DEFAULT_RPC_SERIALIZE_VERSION));
(1) specifically, and not just (>0). If a value is passed that the node doesn’t understand, it should complain, because it means the user is asking to serialize in a way it does not know about.
Removing label “Needs backport”. This was backported in f26dab7 and 21ccb9f
Still needs release notes.
Labels
RPC/REST/ZMQ
Milestone
0.13.2