Add option to return non-segwit serialization via rpc #9194

Merged
merged 2 commits into from Dec 6, 2016

Projects

None yet

10 participants

@instagibbs
Contributor
instagibbs commented Nov 20, 2016 edited

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.

@instagibbs
Contributor

I also think this is a PR worthy of backport.

@gmaxwell
Member

Concept ACK, and I also think it would be worth considering for backport.

@sipa
Member
sipa commented Nov 20, 2016
@instagibbs
Contributor

@sipa Version argument? I'm willing to put in more work if there are better ideas.

@luke-jr
Member
luke-jr commented Nov 20, 2016 edited

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...)

@gmaxwell
Member

Turn the existing boolean argument into something else instead? true/false/stripped

@instagibbs
Contributor

@luke-jr (may be misunderstanding you) older libraries/nodes expect older serialization, not witness serialization minus signatures?

@sipa
Member
sipa commented Nov 20, 2016
@luke-jr
Member
luke-jr commented Nov 20, 2016

@instagibbs "Older serialization" is what you get when you strip signatures... And if they don't care about the signatures, then they don't care about any of the signatures.

@sipa scriptSigs are no more or less useful to strip than segwit signatures.

@sipa
Member
sipa commented Nov 20, 2016
@sdaftuar
Contributor

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.

@luke-jr
Member
luke-jr commented Nov 20, 2016

@sipa If they want the txids, they should be using the verbose mode in the first place, no? And if they actually need the signatures, they should need the segwit signatures as well...

@instagibbs
Contributor
instagibbs commented Nov 20, 2016 edited

@sdaftuar in the interest of doing a command line level argument I'm going to audit how many RPC points would need to know about a flag. This exercise also may come in handy when it comes to switching over wallet functionality to segwit by default, which I'm imagining will be a similar mechanism. I also think that any time we have to change serialization in the future, we can ratchet whatever argument we have, and deprecate older versions if we can't trivially support them.

@sipa
Member
sipa commented Nov 20, 2016
@instagibbs
Contributor
instagibbs commented Nov 21, 2016 edited

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.

src/core_write.cpp
@@ -116,9 +116,9 @@ string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDecode)
return str;
}
-string EncodeHexTx(const CTransaction& tx)
+string EncodeHexTx(const CTransaction& tx, const bool fNonWitnessTx)
@instagibbs
instagibbs Nov 21, 2016 edited Contributor

Perhaps a serialize flag directly here is more appropriate and would effortlessly extend if we have additional serialization flags.

@gmaxwell
gmaxwell Nov 22, 2016 Member

Yes, I think thats a good idea-- and consistent with how things are done for p2p.

@instagibbs instagibbs changed the title from Add flags to getrawtransaction and getblock to return non-segwit seri… to Add option to return non-segwit serialization via rpc Nov 22, 2016
@instagibbs
Contributor

Updated EncodeHexTx to take serialization flag directly, gettransaction["hex"] is now affected, and added more testing.

@sdaftuar
Contributor

As mentioned before and on IRC, I think if we're adding a command line argument to change the serialization for RPC calls, then for consistency's sake we ought to do the same for REST and ZMQ. Looks to me like just a couple additional call sites would be affected.

@instagibbs
Contributor

updated as per @sdaftuar's reasonable request

src/rest.cpp
@@ -228,7 +228,7 @@ static bool rest_block(HTTPRequest* req,
return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
}
- CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION);
+ CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | (GetArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0) ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
@sipa
sipa Nov 22, 2016 Member

Can you extract out this conditional expression into a utility function?

@instagibbs
instagibbs Nov 23, 2016 Contributor

done

src/main.cpp
@@ -3559,6 +3559,14 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
return commitment;
}
+int RPCSerializationFlags()
@sipa
sipa Nov 23, 2016 Member

Unsure this belongs in main. Maybe in util?

@jonasschnelli
jonasschnelli Nov 23, 2016 Member

This should be move from main.cpp to server.cpp or core_write.cpp.

@instagibbs
instagibbs Nov 23, 2016 Contributor

moved to server.cpp

src/rpc/blockchain.cpp
@@ -697,6 +697,7 @@ UniValue getblock(const JSONRPCRequest& request)
"\nArguments:\n"
"1. \"hash\" (string, required) The block hash\n"
"2. verbose (boolean, optional, default=true) true for a json object, false for the hex encoded data\n"
+ "3. nonwit (boolean, optional, default=false) whether to return a non-witness block in non-verbose mode\n"
@jonasschnelli
jonasschnelli Nov 23, 2016 Member

This should go away because it seems to be no longer in use.

@instagibbs
instagibbs Nov 23, 2016 Contributor

fixed thanks

qa/rpc-tests/segwit.py
+ assert(self.nodes[2].getblock(block[0], False) != self.nodes[0].getblock(block[0], False))
+ assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
+ for i in range(len(segwit_tx_list)):
+ #Coinbase can not be a segwit transcation
@sdaftuar
sdaftuar Nov 28, 2016 edited Contributor

I'm confused by this comment -- after segwit activates, the coinbase transaction will generally have a witness, namely the nonce (assuming there's a witness commitment).

qa/rpc-tests/segwit.py
+ assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
+ for i in range(len(segwit_tx_list)):
+ #Coinbase can not be a segwit transcation
+ if "coinbase" in segwit_tx_list[i]:
@sdaftuar
sdaftuar Nov 28, 2016 Contributor

Isn't segwit_tx_list a list of txids?

@instagibbs
Contributor

rebased.

@afk11 afk11 referenced this pull request in bitcoinjs/bitcoinjs-lib Dec 1, 2016
Closed

SegWit: support legacy Transaction export #688

src/core_io.h
@@ -25,7 +25,7 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN
// core_write.cpp
std::string FormatScript(const CScript& script);
-std::string EncodeHexTx(const CTransaction& tx);
+std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
@sdaftuar
sdaftuar Dec 1, 2016 Contributor

extra text?

@instagibbs
instagibbs Dec 1, 2016 Contributor

argh, kdiff. fixing

@jonasschnelli jonasschnelli added this to the 0.14.0 milestone Dec 1, 2016
@instagibbs
Contributor

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

@instagibbs
Contributor

backport tag @fanquake ?

@jtimon
Contributor
jtimon commented Dec 1, 2016

utACK ebe6c30

@sipa
Member
sipa commented Dec 1, 2016

utACK ebe6c30

qa/rpc-tests/segwit.py
+ assert(self.nodes[2].getrawtransaction(segwit_tx_list[i]) != self.nodes[0].getrawtransaction(segwit_tx_list[i]))
+ assert(self.nodes[1].getrawtransaction(segwit_tx_list[i], 0) == self.nodes[2].getrawtransaction(segwit_tx_list[i]))
+ assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) != self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
+ assert(self.nodes[1].getrawtransaction(segwit_tx_list[i]) == self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
@sdaftuar
sdaftuar Dec 1, 2016 Contributor

Perhaps the test should explicitly test the correctness of the serializations, rather than just comparing output against each other? (Maybe just the node0/legacy-serialization case, since I presume something would break if the rpcserializationversion=1 handling were busted.)

@sdaftuar
sdaftuar Dec 1, 2016 Contributor

FYI this did the trick for me:

diff --git a/qa/rpc-tests/segwit.py b/qa/rpc-tests/segwit.py
index a618aec..e8a5512 100755
--- a/qa/rpc-tests/segwit.py
+++ b/qa/rpc-tests/segwit.py
@@ -219,10 +219,13 @@ class SegWitTest(BitcoinTestFramework):
         assert(self.nodes[2].getblock(block[0], False) !=  self.nodes[0].getblock(block[0], False))
         assert(self.nodes[1].getblock(block[0], False) ==  self.nodes[2].getblock(block[0], False))
         for i in range(len(segwit_tx_list)):
+            from test_framework.mininode import FromHex
+            tx = FromHex(CTransaction(), self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
             assert(self.nodes[2].getrawtransaction(segwit_tx_list[i]) != self.nodes[0].getrawtransaction(segwit_tx_list[i]))
             assert(self.nodes[1].getrawtransaction(segwit_tx_list[i], 0) == self.nodes[2].getrawtransaction(segwit_tx_list[i]))
             assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) != self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
             assert(self.nodes[1].getrawtransaction(segwit_tx_list[i]) == self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
+            assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) == bytes_to_hex_str(tx.serialize_without_witness()))
 
         print("Verify witness txs without witness data are invalid after the fork")
         self.fail_mine(self.nodes[2], wit_ids[NODE_2][WIT_V0][2], False)
@instagibbs
instagibbs Dec 2, 2016 Contributor

thanks, included!

@sdaftuar
Contributor
sdaftuar commented Dec 1, 2016

"no objection" utACK ebe6c30

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.

@laanwj laanwj modified the milestone: 0.13.2, 0.14.0 Dec 2, 2016
@MarcoFalke
Member

Note: Needs release notes for 0.13.2 after merge.

@laanwj
Member
laanwj commented Dec 5, 2016

Needs rebase.

@instagibbs
Contributor

rebased

@gmaxwell
Member
gmaxwell commented Dec 6, 2016

ACK (however I did not test rest/ZMQ).

@@ -380,6 +380,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-port=<port>", strprintf(_("Listen for connections on <port> (default: %u or testnet: %u)"), Params(CBaseChainParams::MAIN).GetDefaultPort(), Params(CBaseChainParams::TESTNET).GetDefaultPort()));
strUsage += HelpMessageOpt("-proxy=<ip:port>", _("Connect through SOCKS5 proxy"));
strUsage += HelpMessageOpt("-proxyrandomize", strprintf(_("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)"), DEFAULT_PROXYRANDOMIZE));
+ 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));
@sipa
sipa Dec 6, 2016 Member

I think you should say (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.

@laanwj laanwj merged commit 412bab2 into bitcoin:master Dec 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Dec 6, 2016
@laanwj laanwj Merge #9194: Add option to return non-segwit serialization via rpc
412bab2 Adapt ZMQ/rest serialization to take rpcserialversion arg (instagibbs)
bc7ff8d Add option to return non-segwit serialization via rpc (Gregory Sanders)
ed8d693
@laanwj laanwj added a commit to laanwj/bitcoin that referenced this pull request Dec 6, 2016
@instagibbs @laanwj instagibbs + laanwj Add option to return non-segwit serialization via rpc
Github-Pull: #9194
Rebased-From: 835c75a
21ccb9f
@laanwj laanwj added a commit to laanwj/bitcoin that referenced this pull request Dec 6, 2016
@instagibbs @laanwj instagibbs + laanwj Adapt ZMQ/rest serialization to take rpcserialversion arg
Github-Pull: #9194
Rebased-From: ad5c4c9
f26dab7
@laanwj laanwj referenced this pull request Dec 6, 2016
Merged

0.13.2 backports #9264

@MarcoFalke
Member

Removing label "Needs backport". This was backported in f26dab7 and 21ccb9f

Still needs release notes.

@dcousens dcousens referenced this pull request in bitcoinjs/bitcoinjs-lib Dec 15, 2016
Closed

Transaction: add toLegacyBuffer #715

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment