Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 11, 2016

When merged, this will merge #9292 as well.

I created a new pull, so 0.13.2 isn't blocked by this.

@maflcko maflcko added this to the 0.13.2 milestone Dec 11, 2016
@luke-jr
Copy link
Member

luke-jr commented Dec 11, 2016

It looks intentional and desirable to allow setting future serialisation versions. Especially something like 9999 to mean "use the very latest even if not the default".

@sipa
Copy link
Member

sipa commented Dec 11, 2016

@luke-jr Makes sense, but I was assuming that's what the default was.

@gmaxwell
Copy link
Contributor

ACK

if (GetArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) < 0)
return InitError("rpcserialversion must be non-negative.");

if (GetArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) > 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe define a constant for the max valid RPC serialize version?

Copy link
Member Author

@maflcko maflcko Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which would imply the default is not always the max. It is not clear from the comments if this is desired.

Edit: Anyway, fixing this nit is irrelevant for backport. Either we merge/backport this and bikeshed later or we close the pull based on luke-jr's rationale.

@laanwj laanwj merged commit fa615d3 into bitcoin:master Dec 15, 2016
laanwj added a commit that referenced this pull request Dec 15, 2016
fa615d3 [qa] Don't set unknown rpcserialversion (MarcoFalke)
80d073c Complain when unknown rpcserialversion is specified (Pieter Wuille)
@maflcko maflcko deleted the Mf1612-qaSerial branch December 15, 2016 19:23
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 15, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants