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

instagibbs · 2016-11-20T21:02:37Z

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 · 2016-11-20T21:04:04Z

I also think this is a PR worthy of backport.

gmaxwell · 2016-11-20T21:05:33Z

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

sipa · 2016-11-20T21:06:39Z

Sigh, please no extra boolean arguments everywhere :(

instagibbs · 2016-11-20T21:08:47Z

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

luke-jr · 2016-11-20T21:10:10Z

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 · 2016-11-20T21:13:21Z

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

instagibbs · 2016-11-20T21:17:06Z

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

sipa · 2016-11-20T21:21:18Z

@luke-jr For compatibility reasons, I think that's a bad idea. scriptSigs aren't strippable in any useful way. @instagibbs What about a command-line argument to say whether RPC clients support witnesses?

luke-jr · 2016-11-20T21:32:22Z

@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 · 2016-11-20T21:44:48Z

@luke-jr They are, they determine the txid. You can't strip scriptSigs for any clients that expect to be able to compute the txid. Or expect to be able to see the signatures for whatever other purpose, but don't know about segwit.

sdaftuar · 2016-11-20T21:49:07Z

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 · 2016-11-20T21:54:17Z

@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 · 2016-11-20T21:55:15Z

@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 · 2016-11-20T21:56:35Z

@luke-jr Yes, in an ideal world everybody would use everything the way it is designed. But in an ideal world, we'd also have segwit from the start, and we'd never have any compatibility issue at all. Stripping scriptSigs will break software.

instagibbs · 2016-11-21T19:03:44Z

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.

instagibbs · 2016-11-21T19:08:03Z

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)


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

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

instagibbs · 2016-11-22T14:52:50Z

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

sdaftuar · 2016-11-22T18:23:56Z

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 · 2016-11-22T21:57:40Z

updated as per @sdaftuar's reasonable request

sipa · 2016-11-22T22:24:31Z

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


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

jtimon · 2016-12-01T20:43:01Z

utACK ebe6c30

sipa · 2016-12-01T20:59:24Z

utACK ebe6c30a2763a281425de35fd7b2f5dc87f48199

sdaftuar · 2016-12-01T21:13:14Z

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"])


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

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)

thanks, included!

sdaftuar · 2016-12-01T21:32:36Z

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

maflcko · 2016-12-02T15:51:29Z

Note: Needs release notes for 0.13.2 after merge.

laanwj · 2016-12-05T07:12:33Z

Needs rebase.

instagibbs · 2016-12-05T13:24:46Z

rebased

gmaxwell · 2016-12-06T04:29:31Z

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

sipa · 2016-12-06T07:21:08Z

src/init.cpp

@@ -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));


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.

412bab2 Adapt ZMQ/rest serialization to take rpcserialversion arg (instagibbs) bc7ff8d Add option to return non-segwit serialization via rpc (Gregory Sanders)

Github-Pull: bitcoin#9194 Rebased-From: 835c75acaac004c3315395dcd7d1f193dfb9e5da

Github-Pull: bitcoin#9194 Rebased-From: ad5c4c93cae53a2a6f74880ca11b4d788677a378

maflcko · 2016-12-14T11:56:20Z

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

Still needs release notes.

… rpc 412bab2 Adapt ZMQ/rest serialization to take rpcserialversion arg (instagibbs) bc7ff8d Add option to return non-segwit serialization via rpc (Gregory Sanders)

instagibbs force-pushed the nonswserialrpc branch 2 times, most recently from 273f1c2 to 165dd28 Compare November 21, 2016 19:07

instagibbs commented Nov 21, 2016

View reviewed changes

instagibbs force-pushed the nonswserialrpc branch from 165dd28 to dd906e2 Compare November 22, 2016 14:36

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

instagibbs force-pushed the nonswserialrpc branch 2 times, most recently from 676b293 to 0d757f0 Compare November 22, 2016 14:51

instagibbs force-pushed the nonswserialrpc branch 4 times, most recently from 071f146 to 742f1ac Compare November 22, 2016 21:20

sipa reviewed Nov 22, 2016

View reviewed changes

jonasschnelli added the Needs backport label Dec 1, 2016

sdaftuar reviewed Dec 1, 2016

View reviewed changes

instagibbs force-pushed the nonswserialrpc branch from ebe6c30 to ad5c4c9 Compare December 2, 2016 01:15

laanwj modified the milestones: 0.13.2, 0.14.0 Dec 2, 2016

maflcko added the Needs release note label Dec 2, 2016

instagibbs added 2 commits December 5, 2016 07:43

Add option to return non-segwit serialization via rpc

bc7ff8d

Adapt ZMQ/rest serialization to take rpcserialversion arg

412bab2

instagibbs force-pushed the nonswserialrpc branch from ad5c4c9 to 412bab2 Compare December 5, 2016 12:43

sipa reviewed Dec 6, 2016

View reviewed changes

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

sipa mentioned this pull request Dec 6, 2016

Complain when unknown rpcserialversion is specified #9292

Merged

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Dec 6, 2016

Add option to return non-segwit serialization via rpc

21ccb9f

Github-Pull: bitcoin#9194 Rebased-From: 835c75acaac004c3315395dcd7d1f193dfb9e5da

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Dec 6, 2016

Adapt ZMQ/rest serialization to take rpcserialversion arg

f26dab7

Github-Pull: bitcoin#9194 Rebased-From: ad5c4c93cae53a2a6f74880ca11b4d788677a378

laanwj mentioned this pull request Dec 6, 2016

0.13.2 backports #9264

Merged

maflcko removed the Needs backport label Dec 14, 2016

dcousens mentioned this pull request Dec 15, 2016

Transaction: add toLegacyBuffer bitcoinjs/bitcoinjs-lib#715

Closed

rktoomey mentioned this pull request Feb 3, 2017

bitcoinj fails to deserialize the new 0.13+ block format from bitcoind bitcoinj/bitcoinj#1336

Open

sipa removed the Needs release note label Aug 14, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

instagibbs commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 20, 2016

Uh oh!

gmaxwell commented Nov 20, 2016

Uh oh!

sipa commented Nov 20, 2016 via email

Uh oh!

instagibbs commented Nov 20, 2016

Uh oh!

luke-jr commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Nov 20, 2016

Uh oh!

instagibbs commented Nov 20, 2016

Uh oh!

sipa commented Nov 20, 2016 via email

Uh oh!

luke-jr commented Nov 20, 2016

Uh oh!

sipa commented Nov 20, 2016 via email

Uh oh!

sdaftuar commented Nov 20, 2016

Uh oh!

luke-jr commented Nov 20, 2016

Uh oh!

instagibbs commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 20, 2016 via email

Uh oh!

instagibbs commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 22, 2016

Uh oh!

sdaftuar commented Nov 22, 2016

Uh oh!

instagibbs commented Nov 22, 2016

Uh oh!

sipa Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

instagibbs Nov 23, 2016

Choose a reason for hiding this comment

Uh oh!

jtimon commented Dec 1, 2016

Uh oh!

sipa commented Dec 1, 2016

Uh oh!

sdaftuar Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

sdaftuar Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

instagibbs Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Dec 1, 2016

Uh oh!

maflcko commented Dec 2, 2016

Uh oh!

laanwj commented Dec 5, 2016

Uh oh!

instagibbs commented Dec 5, 2016

Uh oh!

instagibbs commented Nov 20, 2016 •

edited

Loading

luke-jr commented Nov 20, 2016 •

edited

Loading

instagibbs commented Nov 20, 2016 •

edited

Loading

instagibbs commented Nov 21, 2016 •

edited

Loading

instagibbs Nov 21, 2016 •

edited

Loading