Clarification of how to construct the PaymentRequest signature #41

Merged
merged 3 commits into from May 20, 2014

Projects

None yet

7 participants

@harding
Contributor
harding commented Apr 1, 2014

Slightly re-worded description of the signature field in PaymentRequest

The previous phrasing confused me and I had to check the Bitcoin Core source code to see what it was expecting for a signature.

@harding harding Update bip-0070.mediawiki
Slightly re-worded description of the signature field in PaymentRequest

The previous phrasing confused me and I had to check the Bitcoin Core source code to see what it was expecting for a signature.
4081d3c
@rnicoll
Contributor
rnicoll commented Apr 26, 2014

In principal looks good. Could "zero-byte" be "empty string" perhaps, though?

@harding harding Update bip-0070.mediawiki
Change "zero-byte placeholder" to "empty string"
3cbf3d7
@harding
Contributor
harding commented Apr 27, 2014

@rnicoll Changed to "empty string" per your suggestion. Thanks!

@patricklodder

Looks good.

This matches current implementation in paymentrequestplus.cpp as well now.

@schildbach
Contributor

I'm not sure if its relevant, but the empty string should be an "empty byte string" (the signature is composed of bytes rather than chars).

@rnicoll
Contributor
rnicoll commented Apr 27, 2014

Ah... in which case my reading of the current implementation is that because an empty std::string is a character array of length 1 containing a NULL value (terminator), that's then cast into a byte array of length 1, containing the value 0.

Can someone check my working? If we have a spec/implementation mis-match not sure which is better to change in this case.

@schildbach
Contributor

Can't tell about C++. The Java (bitcoinj) implementation uses
paymentRequest.setSignature(ByteString.EMPTY);
and ByteString is not type compatible to String. ByteString is a protobuf specific class, since Java does not know about byte strings, only byte arrays.

The BIP70 spec defines a signature as
optional bytes signature = 5;
I wonder how character strings fit into this picture, even if empty. I'd prefer if the wording of the spec stays consistent and uses byte strings (or byte arrays).

@harding
Contributor
harding commented Apr 27, 2014

The Python code I wrote used an empty string: request.signature = ""

I ran protoc and looked at the objects it created for each of the three supported languages, and it handles the byte-string differently than a regular string in each language. This is most apparent in Java, as @schildbach pointed out, where it types the signature field as a ByteString:

private com.google.protobuf.ByteString signature_;

In C++ and Python, it creates the field as a string but converts it into a byte array (if necessary) when it goes to serialize it. A few more details are in this StackExchange answer.

I thinking that maybe we should say, "empty string (C++/Python) or empty ByteString (Java)." This provides information for programmers of the three languages supported by Google protobuf and also a flag to users of third-party protobuf compilers that they might have to check the code to see what data type is expected.

If there are no objections to that phrasing, I'll change it tomorrow (April 28th UTC).

@gavinandresen
Member

@rnicoll : empty strings in c++ are certainly not length one, and are not zero-terminated (unless you call .c_str() ).

For wording, how about just "The signature field must be set to it's default (empty) value before serialization and signing."

@harding
Contributor
harding commented Apr 27, 2014

@gavinandresen The problem I had which led to this pull request is that I assumed the default value was fine. In the Python protobuf-generated class, at least, that's not the case---you have to explicitly add a line that says, request.signature = "" before you serialize and sign the PaymentRequest. (My research indicated that protobuf does not let you set an empty default value for optional parameters because it uses a default empty value internally to indicate that the field hasn't been set and so shouldn't be serialized.)

@schildbach
Contributor

Indeed, this table explains very nicely how protobuf types map to language types:
https://developers.google.com/protocol-buffers/docs/proto?hl=de#scalar

I think we should stay consistent to protobuf language, which uses the word "bytes". We could say "empty bytes". How that maps to the individual language should not be part of this spec.

It's indeed a bit unfortunate that we did not apply the signature to a protobuf without signature field at all (the default). That would have been much more intuitive. But I fear it's too late to change now, given that there are already 4-5 implementations being used.

@harding
Contributor
harding commented Apr 27, 2014

@schildbach Nice table! I wish I found that. :-)

I like the "empty bytes" suggestion, but when I tried it out, I discovered that it's hard to use the term bytes in a sentence to refer to something besides actual bytes without adding confusing qualifiers or ambiguity. For example:

Before serialization, the signature field must be set to an empty "bytes" value.

Before serialization, the signature field must be set to an empty value of the Protocol Buffers "bytes" type.

I was thinking that it might be better to just say "empty value" but also better explain the expected output so implementers can research how to achieve that output if "empty value" isn't sufficient information:

Before serialization, the signature field must be set to an empty value so that the field is included in the signed PaymentRequest hash but contains no data.

@voisine
Contributor
voisine commented Apr 27, 2014

"Before serialization, the signature field must be set to a
length-delimited field of length zero."

Using the nomenclature in the protobuf documentation:
https://developers.google.com/protocol-buffers/docs/encoding#structure

Aaron

There's no trick to being a humorist when you have the whole government
working for you -- Will Rodgers

On Sun, Apr 27, 2014 at 1:12 PM, David A. Harding
notifications@github.comwrote:

@schildbach https://github.com/schildbach Nice table! I wish I found
that. :-)

I like the "empty bytes" suggestion, but when I tried it out, I discovered
that it's hard to use the term bytes in a sentence to refer to something
besides actual bytes without adding confusing qualifiers or ambiguity. For
example:

Before serialization, the signature field must be set to an empty "bytes"
value.

Before serialization, the signature field must be set to an empty value of
the Protocol Buffers "bytes" type.

I was thinking that it might be better to just say "empty value" but also
better explain the expected output so implementers can research how to
achieve that output if "empty value" isn't sufficient information:

Before serialization, the signature field must be set to an empty value so
that the field is included in the signed PaymentRequest hash but contains
no data.


Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bips/pull/41#issuecomment-41507480
.

@harding
Contributor
harding commented Apr 27, 2014

@voisine isn't "a length-delimited field" a description of the format after serialization, not what should be done "before serialization"?

Taking your meaning, we could include the expected serialized output, which is 0x2a 0x00. For example:

Before serialization, the signature field must be set to an empty value so that the field is included in the signed PaymentRequest hash but contains no data (that is, the PaymentRequest's final two bytes are 0x2a 0x00).

That's a bit wordy, but it certainly makes debugging easy. What do you think?

@gavinandresen
Member

@schildbach : I disagree that it is too late to change, this BIP is still 'Draft'. I'm certainly willing to change the reference implementation to omit the signature field before verification (and change the sample code to match). The fact that it sets it to an empty byte array instead is, I think, clearly a mistake.

@schildbach
Contributor

@gavinandresen That's brave. It would require a coordinated release between bitcoin-qt, Bitcoin Wallet and BitPay, plus maybe others I'm not aware of. Can we do that?

@mikehearn
Contributor

Saying "set it to an empty byte array" seems straightforward enough to me. I don't think anyone has actually had any problems with implementing this part of the spec.

@rnicoll
Contributor
rnicoll commented Apr 28, 2014

As long as the implementation matches that, happy with "set it to an empty byte array". Does feel like it's low value to change now.

@harding
Contributor
harding commented Apr 28, 2014

@mikehearn I spent a couple hours debugging, posting to the forum, and reading Bitcoin Core's source code before figuring out what was expected in the signature field.

I'm not a particularly proficient programmer, which probably accounted for a large amount of my confusion and wasted time, but I also thought that rephrasing the signature field description on BIP70 could save other people some of that hassle.

I did initially try the default @gavinandresen is proposing, so that would certainly be nice for new implementers. However, it seems sufficient to me to just make the BIP70 signature field description clearer using something like the patch in thiss pull request.

I don't really care whether we use "empty byte array", "empty string", "empty 'bytes' value", or whatever else. I think the important part is what comes before that: "Before serialization, the signature field must be set to..." (emphesis added)

Of the current rephrase proposals, my favorite is,

Before serialization, the signature field must be set to an empty value so that the field is included in the signed PaymentRequest hash but contains no data.

@mikehearn
Contributor

That looks good to me.

@rnicoll
Contributor
rnicoll commented Apr 28, 2014

Sounds good

@gavinandresen
Member

ACK for "Before serialization, the signature field must be set to an empty value so that the field is included in the signed PaymentRequest hash but contains no data."

(no consensus on changing the spec and implementations at this point to make it perfect).

@harding harding Update bip-0070.mediawiki
Revised final sentence of signature field description.
9178386
@harding
Contributor
harding commented Apr 28, 2014

Pull request updated to use revised final sentence for the signature field.

@gavinandresen gavinandresen merged commit b7e5b7d into bitcoin:master May 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment