crypto: add SignatureLength constant and use it everywhere

[jpeletier]

Also added PubkeyLength and DigestLength and RecoveryIDOffset, to avoid harcoded numbers

[gballet]

LGTM for the Whisper part.

[karalabe]

I don't really agree with this change. The 65 bytes is not something that's configurable or makes sense to separate out into a global constant. It's essentially defined by the cryptographic primitive of secp256k1, so hard coding 64 might make the code cleaner to read really.

Anyway, @fjl @holiman please chip in your thoughts.

[holiman]

I agree, this is not a 'parameter' which can be configured, so I don't see the reason to modify this.

[jpeletier]

@holiman @karalabe I agree this is not a configurable parameter, but rather is about all the code around that sizes up slices with 65 (x:= make([]byte, 65) and really is a way of expressing you are making room for a ECDSA signature from the crypto package and avoid making mistakes. For veterans this is straightforward, not so for newcomers that want to understand the code.

sig:=make([]byte, 65) means dude is creating a 65 byte slice named "sig". Maybe a signature but what?
sig:=make([]byte, crypto.SignatureLength) means dude is making some space for a signature coming from the crypto package. Let's go to that package to see what that is about.

math.Pi is even less configurable than a signature length, but people don't put 3.1415 all over the place.

I would ask you to reconsider, please don't close the issue without allowing some comments from the contributor. In my work for the feeds module in Swarm I had to define my own constant so I don't have to put 65: crypto.SignatureLength clearly expresses I am making some space for an Ethereum signature. I know their size won't change in 100 years, but it is about making it understandable for people who are not so familiar.

Perhaps some parts can be tweaked in the PR so not everything is as I put it. Can we find some middle ground?

Thanks

[nolash]

The 65 bytes is not something that's configurable

Doesn't that then reinforce the argument that other packages operating on signatures shouldn't seem to have arbitrarily chosen a number, but instead rely on a truth value chaperoned by the package that provides the functionality?

[jpeletier]

@holiman @karalabe can we reconsider? Thanks.

[fjl]

This is a nice change, but unfortunately breaks the nocgo build. Please define the constants in package crypto instead of crypto/secp256k1.

[jpeletier]

@fjl modified this to leave crypto/secp256k1 untouched. Please let me know

[holiman]

I'm not a fan, it adds a level of indirection making it more difficult for a programmer to see explicitly what the lengths and offsets are.

[jpeletier]

magic number 65 in the middle of a code file does not make sense for a newcomer. crypto.SignatureLength is kind of self-explanatory and helps document the code. Any IDE will give you the actual value if you hover over a constant if you want to know the value. In any case, a programmer should not care about the actual value of the constant, just use it.

[fjl]

Rebased on master

[fjl]

Not really sure what happened here, but GitHub can't merge this PR anymore. Will reopen this as a new PR.