accounts, core, crypto, internal: use canonical sigs internally #3455

karalabe · 2016-12-16T12:28:52Z

Ethereum uses the secp256k1 curve cryptography, which has it's own "canonical" signature representation. For the scope of this PR, we only care about the final byte (value of V) being 0 or 1 canonically. Both our crypto library as well as our AccountManager provide methods for generating these signatures.

However, Ethereum transactions themselves are not signed using the canonical representation, rather the yellow paper specifies adding 27 to the V value. This cause both crypto and AccountManager packages gaining additional methods SingEthereum and SignEthereumWithPassphrase. The problem is that anyone using our code as a library now needs to be aware of the crypto differences to use the methods properly.

This issue is further complicated by EIP158 which introduced chain IDs, which are yet another different schemes about how to transform the V value to do chain replay protections. Opposed to the plain and "Ethereum" style signatures, these are hidden from the user even at a library level. However now there's yet again a mental note people need to be aware of.

To address this increasing signature complexity, this PR discards all notion of "different" signature types at the library level. Both the crypto and accounts package is reduced to only be able to produce plain canonical secp256k1 signatures. This makes the crpyto APIs much cleaner, simpler and harder to abuse.

To handle the Ethereum yellow paper or Ethereum EIP158 style signature transforms, all the details have been moved out to the API edges and only executed when a particular signature format is needed. E.g. the Frontier and Homestead signers take a canonical signature and transform them into the canonical format, whereas the EIP158 signer takes again the canonical signature and transforms it according to our chain ID. This allows users to call a single Sign method and have it behave correctly irrelevant where the signature is used.

Similarly the RPC API needed the plain -> yellow papers transforms implemented there too.

mention-bot · 2016-12-16T12:28:53Z

@karalabe, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bas-vk, @obscuren and @Gustav-Simonsson to be potential reviewers.

fjl · 2016-12-16T14:50:35Z

internal/ethapi/api.go

@@ -1113,12 +1117,19 @@ func (s *PublicTransactionPoolAPI) SendRawTransaction(ctx context.Context, encod
 // Sign calculates an ECDSA signature for:
 // keccack256("\x19Ethereum Signed Message:\n" + len(message) + message).
 //
+// Note, the produced signature conforms to the Ethereum yellow paper spec where
+// the V parameter must be 27 or 28 irrelevant of any ChainID features.
+//


Instead of using this sentence ("conforms to the Ethereum yellow paper spec...") please document that the format of the resulting signature is R, S, V where V is 0 or 1. There is no need to mention ChainID here.

fjl · 2016-12-16T14:52:18Z

accounts/account_manager.go

+// can be decrypted with the given passphrase. The produced signature is in the
+// canonical secp256k1 format (V = 0 or 1), which can be transformed into the
+// proper Ethereum signature accoring to the yellow paper within the transaction
+// signer.


Please remove the part where it says how the signature could be transformed by the caller. Any caller can do any transformation on the result. "the transaction signer" is one of the callers, but I'm not sure that people reading the docs on godoc would know where/what that is.

Maybe also add the remark about R, S, V format here. There isn't really a canonical format for secp256k1 signatures.

Maybe we should add a section in the crypto package docs that explains the formats and how they relate to bitcoin signatures.

fjl · 2016-12-16T14:59:31Z

internal/ethapi/api.go

@@ -321,6 +324,9 @@ func (s *PrivateAccountAPI) Sign(ctx context.Context, message string, addr commo
 // hash = keccak256("\x19Ethereum Signed Message:\n"${message length}${message})
 // addr = ecrecover(hash, signature)
 //
+// Note, the signature must conform to the Ethereum yellow paper spec where the V
+// parameter must be 27 or 28 irrelevant of any ChainID features.


No need to mention ChainID. It's sufficient to say that the accepted format is R, S, V where V must be 27 or 28 for legacy reasons.

fjl · 2016-12-16T14:59:59Z

internal/ethapi/api.go

@@ -312,6 +314,7 @@ func (s *PrivateAccountAPI) Sign(ctx context.Context, message string, addr commo
 	if err != nil {
 		return "0x", err
 	}
+	signature[64] += 27 // SignWithPassphrase uses canonical secp256k1 signatures (v = 0 or 1), transform to yellow paper


comment could be shorter

fjl · 2016-12-16T15:02:18Z

crypto/crypto.go

@@ -181,7 +184,7 @@ func ValidateSignatureValues(v byte, r, s *big.Int, homestead bool) bool {
 	if s.Cmp(secp256k1.N) >= 0 {
 		return false
 	}
-	if r.Cmp(secp256k1.N) < 0 && (vint == 27 || vint == 28) {
+	if r.Cmp(secp256k1.N) < 0 && (vint == 0 || vint == 1) {
 		return true
 	} else {
 		return false


This can be

return r.Cmp(secp256k1.N) < 0 && s.Cmp(secp256k1.N) < 0 && (v == 0 || v == 1)

Please move changes around ValidateSignatureValues into their own commit.

karalabe · 2016-12-19T09:20:50Z

@fjl I've fixed up all issues in the first commit and also moved anything unrelated to ValidateSignatureValues into there. The second commit remained only the validation updates. PTAL

fjl · 2016-12-19T23:49:49Z

It looks better now, but I still stand by my comment that there is no such thing as "canonical secp256k1 format".

The format of a signature is independent of the curve. An ECDSA signature is a tuple of two numbers, R and S. For secp25k1 signatures, the additional V value is required to recover the correct public key. It is not required for plain signature verification where the expected public key is known.

The signature format we use everywhere is just R, S, V concatenated. This format is not standard or canonical, it's just what we use. Signatures are usually encoded using ASN.1 DER, but we don't do that. Bitcoin has a 'compact' signature format, which is V, R, S with V + 27 for arbitrary reasons.

So again, please remove the phrase "canonical secp256k1 format" from comments. If you want to document the format, you can say "R, S, V format where V is 0 or 1". That's unambiguous and everyone
will know what you mean. You can also define the format somewhere (e.g. package crypto docs) and then refer to it.

karalabe · 2016-12-20T09:28:19Z

PTAL. No, I'm not going to flatten that last commit into the first two because it would need to split up in between the two commits to avoid rebase conflicts.

…andling (ethereum#3455) To address increasing complexity in code that handles signatures, this PR discards all notion of "different" signature types at the library level. Both the crypto and accounts package is reduced to only be able to produce plain canonical secp256k1 signatures. This makes the crpyto APIs much cleaner, simpler and harder to abuse.

karalabe requested review from fjl, holiman and obscuren December 16, 2016 12:36

karalabe added core labels Dec 16, 2016

karalabe added this to the 1.5.6 milestone Dec 16, 2016

fjl requested changes Dec 16, 2016

View reviewed changes

karalabe force-pushed the only-use-canonical-signatures branch 2 times, most recently from 1191c79 to 2b120bd Compare December 19, 2016 09:18

karalabe force-pushed the only-use-canonical-signatures branch from 2b120bd to ed16403 Compare December 19, 2016 10:03

karalabe force-pushed the only-use-canonical-signatures branch from ed16403 to 6eca171 Compare December 20, 2016 08:48

karalabe mentioned this pull request Dec 20, 2016

Account management API polishes #3417

Merged

fjl approved these changes Dec 20, 2016

View reviewed changes

karalabe added 3 commits January 4, 2017 13:28

accounts, core, crypto, internal: use canonical sigs internally

b746cd7

core, crypto: use canonical signature validation

709cb15

accounts, core, crypto: fun with docs to make things explicit

e1db48e

karalabe force-pushed the only-use-canonical-signatures branch from 482377c to e1db48e Compare January 4, 2017 12:30

fjl merged commit 08eea0f into ethereum:master Jan 5, 2017

fjl removed the review label Jan 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accounts, core, crypto, internal: use canonical sigs internally #3455

accounts, core, crypto, internal: use canonical sigs internally #3455

karalabe commented Dec 16, 2016

mention-bot commented Dec 16, 2016

fjl Dec 16, 2016 •

edited

fjl Dec 16, 2016

fjl Dec 17, 2016 •

edited

fjl Dec 17, 2016 •

edited

fjl Dec 16, 2016 •

edited

fjl Dec 16, 2016

fjl Dec 16, 2016

karalabe commented Dec 19, 2016

fjl commented Dec 19, 2016 •

edited

karalabe commented Dec 20, 2016

accounts, core, crypto, internal: use canonical sigs internally #3455

accounts, core, crypto, internal: use canonical sigs internally #3455

Conversation

karalabe commented Dec 16, 2016

mention-bot commented Dec 16, 2016

fjl Dec 16, 2016 • edited

Choose a reason for hiding this comment

fjl Dec 16, 2016

Choose a reason for hiding this comment

fjl Dec 17, 2016 • edited

Choose a reason for hiding this comment

fjl Dec 17, 2016 • edited

Choose a reason for hiding this comment

fjl Dec 16, 2016 • edited

Choose a reason for hiding this comment

fjl Dec 16, 2016

Choose a reason for hiding this comment

fjl Dec 16, 2016

Choose a reason for hiding this comment

karalabe commented Dec 19, 2016

fjl commented Dec 19, 2016 • edited

karalabe commented Dec 20, 2016

fjl Dec 16, 2016 •

edited

fjl Dec 17, 2016 •

edited

fjl Dec 17, 2016 •

edited

fjl Dec 16, 2016 •

edited

fjl commented Dec 19, 2016 •

edited