test: refactor: introduce `generate_keypair` helper with WIF support

theStack commented at 0:07 am on May 24, 2023: contributor

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).

DrahtBot commented at 0:07 am on May 24, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stratospher, instagibbs, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on May 24, 2023

glozow added the label Good First Review on Jun 7, 2023

in test/functional/test_framework/wallet_util.py:123 in 25f19c0123 outdated

123+    interaction."""
124+    privkey = ECKey()
125+    privkey.generate(compressed)
126+    pubkey = privkey.get_pubkey().get_bytes()
127+    if wif:
128+        privkey = bytes_to_wif(privkey.get_bytes(), privkey.is_compressed)

kevkevinpal commented at 11:15 pm on June 15, 2023:

just a question but why use privkey.is_compressed here and not the compressed from the function args?

theStack commented at 3:42 pm on June 19, 2023:

Thanks, good eye! Agree that simply using compressed is preferred, done.

kevkevinpal approved

kevkevinpal commented at 11:45 pm on June 15, 2023: contributor

code review ACK 25f19c012395ae585e919686170d89aed063c30c

added one question

also ran grep for generate_wif_key in ./test on this commit 25f19c012395ae585e919686170d89aed063c30c and seems like all them are gone

test: refactor: introduce `generate_keypair` helper with WIF support

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).

1a572ce7d6

theStack force-pushed on Jun 19, 2023

theStack commented at 3:45 pm on June 19, 2023: contributor

Addressed review comment #27733 (review) and rebased on master – with the merge of #25634 there was another ECKey instance in wallet_blank.py that could be replaced with the new generate_keypair helper.

Changes since the initial version can be reviewed via $ git range-diff 25f19c01...1a572ce7.

stratospher commented at 8:19 am on June 20, 2023: contributor

ACK 1a572ce7. neat to have this since keypair generation is done in lots of places.

DrahtBot requested review from kevkevinpal on Jun 20, 2023

fanquake removed review request from kevkevinpal on Jun 20, 2023

fanquake requested review from instagibbs on Jun 20, 2023

instagibbs commented at 1:38 pm on June 20, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27733/commits/1a572ce7d6e2b8282c6ad457cf8ecd2cf5ab7fd6

There are 3 remaining uses of ECKey outside of this new utility function, each of which are deterministic uses.

DrahtBot removed review request from instagibbs on Jun 20, 2023

DrahtBot requested review from kevkevinpal on Jun 20, 2023

kevkevinpal commented at 3:27 pm on June 20, 2023: contributor

reACK 1a572ce

DrahtBot removed review request from kevkevinpal on Jun 20, 2023

fanquake merged this on Jun 21, 2023

fanquake closed this on Jun 21, 2023

theStack deleted the branch on Jun 21, 2023

in test/functional/test_framework/wallet_util.py:114 in 1a572ce7d6

110@@ -114,8 +111,14 @@ def bytes_to_wif(b, compressed=True):
111         b += b'\x01'
112     return byte_to_base58(b, 239)
113 
114-def generate_wif_key():
115-    # Makes a WIF privkey for imports
116-    k = ECKey()
117-    k.generate()
118-    return bytes_to_wif(k.get_bytes(), k.is_compressed)
119+def generate_keypair(compressed=True, wif=False):

maflcko commented at 11:12 am on June 21, 2023:

Nit to force named args:

0def generate_keypair(*, compressed=True, wif=False):

sidhujag referenced this in commit 0e8abb33ef on Jun 21, 2023

joostjager referenced this in commit e100018cbe on Jun 26, 2023

luke-jr referenced this in commit d2548cd38a on Oct 20, 2023

bitcoin locked this on Jun 20, 2024

test: refactor: introduce generate_keypair helper with WIF support #27733

Reviews

Conflicts

test: refactor: introduce `generate_keypair` helper with WIF support #27733