test: use address_to_scriptpubkey instead of RPC call #27349

ismaelsadeeq commented at 10:50 PM on March 27, 2023: member

PR #27269 enables the function address_to_scriptpubkey() to decode all address types and return their corresponding scriptpubkeys. As a result, there is no longer any need to call getaddressinfo or validateaddress RPCs in order to retrieve an address scriptpubkey, as explained in the comments on this pull request (see #27269#pullrequestreview-1353681933 and #27269 (comment)).

Instead of using RPC calls, this update replaces the process of obtaining an address scriptPubkey with the address_to_scriptpubkey method, resulting in improved performance for functional tests.

DrahtBot commented at 10:50 PM on March 27, 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	josibake, theStack

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

DrahtBot added the label Refactoring on Mar 27, 2023

ismaelsadeeq commented at 10:53 PM on March 27, 2023: member

@josibake @theStack @willcl-ark this is PR #27269 follow up, I will appreciate your reviews. Thank you

ismaelsadeeq renamed this:
~~refactor: use address_to_scriptpubkey instead of RPC call~~
test: use address_to_scriptpubkey instead of RPC call
on Mar 27, 2023

ismaelsadeeq force-pushed on Mar 27, 2023

ismaelsadeeq marked this as a draft on Mar 27, 2023

ismaelsadeeq force-pushed on Mar 28, 2023

ismaelsadeeq marked this as ready for review on Mar 28, 2023

ismaelsadeeq marked this as a draft on Mar 28, 2023

in test/functional/test_framework/wallet.py:100 in a83181eda2 outdated

  96 | @@ -101,7 +97,7 @@ def __init__(self, test_node, *, mode=MiniWalletMode.ADDRESS_OP_TRUE):
  97 |              self._scriptPubKey = key_to_p2pk_script(pub_key.get_bytes())
  98 |          elif mode == MiniWalletMode.ADDRESS_OP_TRUE:
  99 |              self._address, self._internal_key = create_deterministic_address_bcrt1_p2tr_op_true()
 100 | -            self._scriptPubKey = bytes.fromhex(self._test_node.validateaddress(self._address)['scriptPubKey'])
 101 | +            self._scriptPubKey = address_to_scriptpubkey(self._address)

josibake commented at 6:51 AM on March 28, 2023:

I think this should be in the second commit, right?

ismaelsadeeq commented at 10:23 AM on March 28, 2023:

Yes.

in test/functional/feature_nulldummy.py:82 in c73a0658c2 outdated

  77 | @@ -77,7 +78,8 @@ def run_test(self):
  78 |          cms = self.nodes[0].createmultisig(1, [self.pubkey])
  79 |          wms = self.nodes[0].createmultisig(1, [self.pubkey], 'p2sh-segwit')
  80 |          self.ms_address = cms["address"]
  81 | -        ms_unlock_details = {"scriptPubKey": self.nodes[0].validateaddress(self.ms_address)["scriptPubKey"],
  82 | +        spk = address_to_scriptpubkey(self.ms_address).hex()
  83 | +        ms_unlock_details = {"scriptPubKey":spk,

josibake commented at 7:17 AM on March 28, 2023:

I'm not sure adding spk as a variable really helps anything here and it does add an extra line to be reviewed. For a refactor, I'd suggest keeping the diff as small and clear as possible

        ms_unlock_details = {"scriptPubKey": address_to_scriptpubkey(self.ms_address).hex(),

ismaelsadeeq commented at 10:24 AM on March 28, 2023:

Thanks.

josibake commented at 7:23 AM on March 28, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27349/commits/c73a0658c2c5c43d4ba2d8dbf2af71abe84b40e8

Nice to see a standard function for getting the scriptPubKey instead of a mix of RPC calls. Removing the RPC call should make the tests run faster, but considering it wasn't used in many places, I doubt it makes a material difference. Left two non-blocking nits.

I also noticed the CI was failing, but when I rebased on master and ran the failing functional test locally, everything passed.

josibake commented at 7:24 AM on March 28, 2023: member

@ismaelsadeeq seems ready for review to me

ismaelsadeeq marked this as ready for review on Mar 28, 2023

ismaelsadeeq force-pushed on Mar 28, 2023

ismaelsadeeq commented at 1:12 PM on March 28, 2023: member

@ismaelsadeeq seems ready for review to me

Ready for review now.

ismaelsadeeq commented at 1:21 PM on March 28, 2023: member

ACK c73a065

Nice to see a standard function for getting the scriptPubKey instead of a mix of RPC calls. Removing the RPC call should make the tests run faster, but considering it wasn't used in many places, I doubt it makes a material difference. Left two non-blocking nits.

I also noticed the CI was failing, but when I rebased on master and ran the failing functional test locally, everything passed.

Thank you for the review Josi I rebased to master recently but it failed again.

josibake commented at 1:47 PM on March 28, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27349/commits/01e0a1679fb1deeae4daf674730de2e1a270139e

thanks for taking the suggestions! CI failure seems unrelated from what I can tell :shrug:

in test/functional/feature_nulldummy.py:81 in 01e0a1679f outdated

  77 | @@ -77,7 +78,7 @@ def run_test(self):
  78 |          cms = self.nodes[0].createmultisig(1, [self.pubkey])
  79 |          wms = self.nodes[0].createmultisig(1, [self.pubkey], 'p2sh-segwit')
  80 |          self.ms_address = cms["address"]
  81 | -        ms_unlock_details = {"scriptPubKey": self.nodes[0].validateaddress(self.ms_address)["scriptPubKey"],
  82 | +        ms_unlock_details = {"scriptPubKey":address_to_scriptpubkey(self.ms_address).hex(),

josibake commented at 1:48 PM on March 28, 2023:

nit: should be a space here

        ms_unlock_details = {"scriptPubKey": address_to_scriptpubkey(self.ms_address).hex(),

ismaelsadeeq commented at 1:49 PM on March 28, 2023:

Thanks

ismaelsadeeq force-pushed on Mar 28, 2023

in test/functional/test_framework/address.py:28 in 19db09fcec outdated

  28 | +)
  29 |  from test_framework.segwit_addr import (
  30 |      decode_segwit_address,
  31 |      encode_segwit_address,
  32 |  )
  33 | -

theStack commented at 3:10 PM on March 28, 2023:

stylistic nit: we usually keep two blank lines after imports, so should rather add another one than remove one here (see also https://peps.python.org/pep-0008/#blank-lines).

ismaelsadeeq commented at 3:47 PM on March 28, 2023:

Thanks :100:

theStack commented at 3:17 PM on March 28, 2023: contributor

Concept ACK, thanks for following up! LGTM, left just one small suggestion below.

refactor: move address_to_scriptpubkey to address.py

The COINBASE_MATURITY constant in blocktools.py is imported in wallet.py.
However, importing address_to_scriptpubkey to blocktools.py will
generate a circular import error. Since the method is related to
addresses, it is best to move it to address.py, which will also
fix the circular import error.

Update imports of address_to_scriptpubkey accordingly.

4142d19d74

refactor: use address_to_scriptpubkey to retrieve addresses scriptpubkey

This commit updates the code by replacing the RPC call used to
decode an address and retrieve its corresponding scriptpubkey
with the address_to_scriptpubkey function. address_to_scriptpubkey
function can now decode all addresses formats, which makes
it more efficient to use.

e47ce42f67

ismaelsadeeq force-pushed on Mar 28, 2023

josibake commented at 4:02 PM on March 28, 2023: member

re-ACK https://github.com/bitcoin/bitcoin/pull/27349/commits/e47ce42f670fc43859c157766b342509ab5916f9

theStack approved

theStack commented at 4:10 PM on March 28, 2023: contributor

ACK e47ce42f670fc43859c157766b342509ab5916f9 🌱

fanquake merged this on Mar 29, 2023

fanquake closed this on Mar 29, 2023

fanquake commented at 11:05 AM on March 29, 2023: member

resulting in improved performance for functional tests.

Generally good to provide some sort of benchmark/indication of the speedup when talking about performance.

sidhujag referenced this in commit d3dab3d890 on Mar 29, 2023

bitcoin locked this on Jun 9, 2024

ismaelsadeeq deleted the branch on Jun 27, 2024