txscript: Force extracted addrs to compressed. #775

dnldd · 2017-07-30T14:38:17Z

~~This requires PR decred/dcrutil#55~~.

Decred's consensus rules require address generation to use a compressed pubkey format, this was hacked
into btcutil.NewAddressSecpPubKey. This commit removes the hack and provides btcutil.NewAddressSecpPubKeyCompressed to enforce the consensus prerequisite

davecgh · 2017-07-31T12:57:35Z

txscript/standard_test.go

+		"b93e1dcdb8a016b49840f8c53bc1eb68a382e97b1482ecad7b148a6909a5c" +
+		"b2e0eaddfb84ccf9744464f82e160bfa9b8b64f9d4c03f999b8643f656b41" +
+		"2a3"))
+


Extra blank line.

davecgh · 2017-07-31T13:02:26Z

rpcserver.go

@@ -5737,30 +5737,21 @@ func handleVerifyMessage(s *rpcServer, cmd interface{}, closeChan <-chan struct{
 	wire.WriteVarString(&buf, 0, "Decred Signed Message:\n")
 	wire.WriteVarString(&buf, 0, c.Message)
 	expectedMessageHash := chainhash.HashB(buf.Bytes())
-	pk, wasCompressed, err := chainec.Secp256k1.RecoverCompact(sig,


This function should be kept the same. Compressed and uncompressed pubkeys result in different addresses and therefore they should not verify.

This was incorrect before due to the hack in NewAddressSecpPubKey, but now that NewAddressSecpPubKey has been fixed, this function should be left the way it was so it is correct.

davecgh · 2017-07-31T13:04:08Z

rpcserver.go

 		activeNetParams.Params)
 	if err != nil {
 		// Again mirror Bitcoin Core behavior, which treats error in
 		// public key reconstruction as invalid signature.
 		return false, nil
 	}
-


Please don't remove this newline. If you'll notice in the majority of the code base there is always a newline before a code block with a comment.

davecgh · 2017-07-31T14:57:21Z

mempool/mempool_test.go

@@ -313,8 +313,7 @@ func newPoolHarness(chainParams *chaincfg.Params) (*poolHarness, []spendableOutp

 	// Generate associated pay-to-script-hash address and resulting payment
 	// script.
-	pubKeyBytes := signPub.SerializeCompressed()
-	payPubKeyAddr, err := dcrutil.NewAddressSecpPubKey(pubKeyBytes,
+	payPubKeyAddr, err := dcrutil.NewAddressSecpPubKeyCompressed(signPub,


Please leave this the way it was. It is calling the function with the compressed bytes, so it's valid, and I prefer not to churn things that don't need to be churned since it makes upstream merges harder due to unnecessary differences.

davecgh · 2017-07-31T14:58:33Z

rpctest/memwallet.go

@@ -553,8 +553,7 @@ func (m *memWallet) ConfirmedBalance() dcrutil.Amount {
 // keyToAddr maps the passed private to corresponding p2pkh address.
 func keyToAddr(key chainec.PrivateKey, net *chaincfg.Params) (dcrutil.Address, error) {
 	pubKey := chainec.Secp256k1.NewPublicKey(key.Public())
-	serializedKey := pubKey.SerializeCompressed()
-	pubKeyAddr, err := dcrutil.NewAddressSecpPubKey(serializedKey, net)
+	pubKeyAddr, err := dcrutil.NewAddressSecpPubKeyCompressed(pubKey, net)


Same here. Please leave it as is since It is calling the function with the compressed bytes already.

davecgh · 2017-07-31T15:36:47Z

txscript/standard_test.go

-	p2pkCompressed2Main, err := dcrutil.NewAddressSecpPubKey(decodeHex("03b0bd"+
-		"634234abbb1ba1e986e884185c61cf43e001f9137f23c2c409273eb16e65"),
+
+	pubkey2, err := chainec.Secp256k1.ParsePubKey(decodeHex("03b0bd" +


Please don't change this. There is no need since they are specifically being given the compressed bytes.

davecgh · 2017-07-31T15:37:18Z

txscript/standard_test.go

+
+	pubkey2, err := chainec.Secp256k1.ParsePubKey(decodeHex("03b0bd" +
+		"634234abbb1ba1e986e884185c61cf43e001f9137f23c2c409273eb16e65"))
+


Extra blank line.

davecgh · 2017-07-31T15:38:11Z

txscript/standard_test.go

@@ -671,27 +698,52 @@ func TestMultiSigScript(t *testing.T) {
 	t.Parallel()

 	//  mainnet p2pk 13CG6SJ3yHUXo4Cr2RY4THLLJrNFuG3gUg
-	p2pkCompressedMain, err := dcrutil.NewAddressSecpPubKey(decodeHex("02192d7"+


Please don't change this. There is no need since they are specifically being given the compressed bytes.

davecgh · 2017-07-31T15:38:20Z

txscript/standard_test.go

 		&chaincfg.MainNetParams)
 	if err != nil {
 		t.Errorf("Unable to create pubkey address (compressed): %v",
 			err)
 		return
 	}
-	p2pkCompressed2Main, err := dcrutil.NewAddressSecpPubKey(decodeHex("03b0bd"+


Please don't change this. There is no need since they are specifically being given the compressed bytes.

davecgh · 2017-07-31T15:39:16Z

txscript/standard_test.go

-		"b93e1dcdb8a016b49840f8c53bc1eb68a382e97b1482ecad7b148a6909a5c"+
-		"b2e0eaddfb84ccf9744464f82e160bfa9b8b64f9d4c03f999b8643f656b41"+
-		"2a3"), &chaincfg.MainNetParams)
+	pubkey3, err := chainec.Secp256k1.ParsePubKey(decodeHex("0411d" +


As above, this could just use newAddressPubKey.

davecgh · 2017-07-31T19:57:59Z

txscript/standard_test.go

 			err)
 		return
 	}
+	p2pkUncompressedMain := newAddressPubKey(pubkey.SerializeCompressed())


davecgh · 2017-07-31T19:58:33Z

txscript/standard_test.go

 			err)
 		return
 	}
+	p2pkUncompressedMain := newAddressPubKey(pubkey.SerializeCompressed())


Please Just pass it the decodeHex result. There is no sense in parsing it just to serialize it and have it reparsed again in the function.

p2pkUncompressedMain := newAddressPubKey(decodeHex("0411db" + "93e1dcdb8a016b49840f8c53bc1eb68a382e97b1482ecad7b148a6909a5cb2" + "e0eaddfb84ccf9744464f82e160bfa9b8b64f9d4c03f999b8643f656b412a3"))

davecgh · 2017-07-31T20:43:48Z

txscript/sign_test.go

-				pkBytes := pk.SerializeUncompressed()
-
-				address, err := dcrutil.NewAddressSecpPubKey(pkBytes,
+				// TODO: this test might have to be removed since consensus rules


These tests shouldn't be removed. Instead, the comment should call out that they are required to be compressed currently due to the workaround in ExtractPkScriptAddrs.

Ultimately, the consensus rules need to be updated so they do not call into a standardness function. Standard rules are intended to be a subset of consensus and invoking them is a mistake.

davecgh

This looks good now. We'll need to make sure to get the dependent btcutil PR in first and update the glide.yaml and glide.lock accordingly before merging.

davecgh · 2017-08-01T02:45:44Z

Please amend the commit title match the PR title so that it properly calls out the package it modifies and conforms to the Model Git Commit Message from the Code Contribution Guidelines.

davecgh · 2017-08-02T00:04:27Z

glide.lock

@@ -1,5 +1,5 @@
-hash: 153a3e6a1f31b2b29a113e953098f2b42a645103ee3e10040b5e03727a7b003c
-updated: 2017-07-26T12:10:25.6313172-05:00
+hash: bfe9054cc320a713e759ae524d46404f5fa085a62466fa172e1dcb4d8d08acbd


This should not change since the glide.yaml didn't change.

decred's consensus rules require address generation to use a compressed pubkey format, this was hacked into NewAddressSecpPubKey. This commit removes the hack and provides NewAddressSecpPubKeyCompressed to enforce the consensus prerequisite

davecgh requested changes Jul 31, 2017

View reviewed changes

dnldd force-pushed the enforcecompressed branch 3 times, most recently from 9cde4ba to 66f0e7b Compare July 31, 2017 19:49

davecgh requested changes Jul 31, 2017

View reviewed changes

dnldd force-pushed the enforcecompressed branch from 66f0e7b to c5474f5 Compare July 31, 2017 21:22

davecgh reviewed Jul 31, 2017

View reviewed changes

davecgh changed the title ~~Replace NewAddressSecpPubKey calls with NewAddressSecpPubKeyCompressed~~ txscript: Force extracted addrs to compressed. Aug 1, 2017

dnldd force-pushed the enforcecompressed branch 2 times, most recently from 525df3a to 8baee3c Compare August 1, 2017 23:58

davecgh reviewed Aug 2, 2017

View reviewed changes

dnldd force-pushed the enforcecompressed branch 3 times, most recently from 83a96b7 to c85f9d6 Compare August 2, 2017 00:23

dnldd force-pushed the enforcecompressed branch from c85f9d6 to 2c1e17b Compare August 2, 2017 00:34

davecgh approved these changes Aug 2, 2017

View reviewed changes

davecgh merged commit 2c1e17b into decred:master Aug 2, 2017

dnldd deleted the enforcecompressed branch August 3, 2017 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txscript: Force extracted addrs to compressed. #775

txscript: Force extracted addrs to compressed. #775

dnldd commented Jul 30, 2017 •

edited

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh Jul 31, 2017

davecgh left a comment

davecgh commented Aug 1, 2017

davecgh Aug 2, 2017


		pubkey2, err := chainec.Secp256k1.ParsePubKey(decodeHex("03b0bd" +
		"634234abbb1ba1e986e884185c61cf43e001f9137f23c2c409273eb16e65"))

txscript: Force extracted addrs to compressed. #775

txscript: Force extracted addrs to compressed. #775

Conversation

dnldd commented Jul 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh left a comment

Choose a reason for hiding this comment

davecgh commented Aug 1, 2017

Choose a reason for hiding this comment

dnldd commented Jul 30, 2017 •

edited