Clarification of how to construct the PaymentRequest signature #41

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.

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

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

Looks good.

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

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

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.

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

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

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

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

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.

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

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

@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?

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

@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?

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.

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.

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

That looks good to me.

Sounds good

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

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