Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented May 1, 2013

This pull request changes a few things in key.h/key.cpp

  • CKey is no longer a kitchen-sink for "anything ECDSA related", but just an object representing a private key.
  • Operations that don't use a private key are moved to CPubKey (which is now a fully-featured object representing a public key).
  • All actual OpenSSL-interaction code is moved to an internal class in key.cpp.
  • Representation-wise, CKey now funcions as CSecret (which is gone) with an fCompressed built-in (so no keeping separate booleans necessary anymore).
  • CKey and CPubKey simply encapsulate static byte arrays, and can be created/moved/copied cheaply.
  • Practical upshot: no need to move key/pubkey data from/to CKey anymore before doing something useful with it.

The reason for writing this is preparing the code to more easily change ECDSA implementation (such as maybe my secp256k1 library at some point), but it seemed useful enough to try to get reviewed and perhaps merged independently of that.

@jgarzik
Copy link
Contributor

jgarzik commented May 1, 2013

Seems correct at first glance. Would prefer init-to-zero over init-to-0xFF but that's nit picking.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b6d07ce7ea07b1b79e96c25d7964f628fc3dbd4e for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa
Copy link
Member Author

sipa commented May 3, 2013

@jgarzik 0x00 is a valid serialized EC point (though not a valid ECDSA public key), so I prefer using a certainly-invalid data there, to trigger more errors in case of some out-of-bounds access.

@jgarzik
Copy link
Contributor

jgarzik commented May 3, 2013

Fair enough. ACK.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b7b49b89d1f5b510c6269bb3d516a5300a53087f for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/fd6e0a58f1b1b4b2ada60ba197ee541f5f0885d4 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa sipa mentioned this pull request May 20, 2013
@sipa
Copy link
Member Author

sipa commented May 30, 2013

Rebased.

jgarzik pushed a commit that referenced this pull request May 30, 2013
@jgarzik jgarzik merged commit e2f4214 into bitcoin:master May 30, 2013
@jgarzik
Copy link
Contributor

jgarzik commented May 30, 2013

Wanted to get this one in sooner rather than later, so that other work may be based on top of it (it stirs a lot of code)

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants