-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: refactor: introduce generate_keypair
helper with WIF support
#27733
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.: privkey = ECKey() privkey.generate() privkey_wif = bytes_to_wif(privkey.get_bytes()) pubkey = privkey.get_pubkey().get_bytes() Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't use `ECPubKey` objects from generated keypairs anywhere). With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.: privkey, pubkey = generate_keypair(wif=True) Note that after this commit, the only direct uses of `ECKey` remain in situations where we want to set the private key explicitly, e.g. in MiniWallet (test/functional/test_framework/wallet.py) or the test for the signet miner script (test/functional/tool_signet_miner.py).
25f19c0
to
1a572ce
Compare
Addressed review comment #27733 (comment) and rebased on master -- with the merge of #25634 there was another Changes since the initial version can be reviewed via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1a572ce. neat to have this since keypair generation is done in lots of places.
ACK 1a572ce There are 3 remaining uses of ECKey outside of this new utility function, each of which are deterministic uses. |
reACK 1a572ce |
k = ECKey() | ||
k.generate() | ||
return bytes_to_wif(k.get_bytes(), k.is_compressed) | ||
def generate_keypair(compressed=True, wif=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit to force named args:
def generate_keypair(*, compressed=True, wif=False):
…per with WIF support 1a572ce test: refactor: introduce `generate_keypair` helper with WIF support (Sebastian Falbesoner) Pull request description: In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.: privkey = ECKey() privkey.generate() privkey_wif = bytes_to_wif(privkey.get_bytes()) pubkey = privkey.get_pubkey().get_bytes() Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't use `ECPubKey` objects from generated keypairs anywhere). With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.: privkey, pubkey = generate_keypair(wif=True) Note that after this commit, the only direct uses of `ECKey` remain in situations where we want to set the private key explicitly, e.g. in MiniWallet (test/functional/test_framework/wallet.py) or the test for the signet miner script (test/functional/tool_signet_miner.py). ACKs for top commit: instagibbs: ACK bitcoin@1a572ce kevkevinpal: reACK [1a572ce](bitcoin@1a572ce) stratospher: ACK 1a572ce. neat to have this since keypair generation is done in lots of places. Tree-SHA512: ceb695ba7b34dc9f65357b55be03e67609e7e13a178083d405284eff4d8d3c5cea4fb0b6632658604a533f38ebfefc33e0c375995cc21ebc7843442ad764287b
In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.: privkey = ECKey() privkey.generate() privkey_wif = bytes_to_wif(privkey.get_bytes()) pubkey = privkey.get_pubkey().get_bytes() Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't use `ECPubKey` objects from generated keypairs anywhere). With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.: privkey, pubkey = generate_keypair(wif=True) Github-Pull: bitcoin#27733 Rebased-From: 1a572ce [partial]
In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.:
Simplify this by providing a new
generate_keypair
helper function that returns the private key either asECKey
object or as WIF-string (depending on the booleanwif
parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't useECPubKey
objects from generated keypairs anywhere).With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.:
Note that after this commit, the only direct uses of
ECKey
remain in situations where we want to set the private key explicitly, e.g. in MiniWallet (test/functional/test_framework/wallet.py) or the test for the signet miner script (test/functional/tool_signet_miner.py).