test: support mainnet base58 in address_to_scriptpubkey #34770

Summary

• Extend test/functional/test_framework/address.py::address_to_scriptpubkey to support mainnet Base58 version bytes:
  • P2PKH: 0
  • P2SH: 5
• Replace assert-based Base58 character validation with explicit ValueError.
• Add explicit ValueError handling for:
  • empty input
  • unsupported Base58 version bytes
  • invalid payload lengths

Motivation

address_to_scriptpubkey supported Bech32 and testnet Base58, but not mainnet Base58 (0, 5), despite being a shared helper in functional tests.

This change closes that gap and makes failure modes explicit and testable.

Tests

• cd test/functional && python3 -m unittest test_framework.address
• test/functional/feature_framework_unit_tests.py
• python3 -m ruff check --select=B006,B008,E101,E401,E402,E701,E702,E703,E711,E713,E714,E721,E722,E742,E743,F401,F402,F403,F404,F405,F406,F407,F541,F601,F602,F621,F631,F632,F811,F821,F822,F823,F841,PLE,W191,W291,W292,W293,W605 test/functional/test_framework/address.py

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Code Review ACK

Reviewed the diff. Clean, well-tested improvement.

• Correctly adds mainnet version bytes (0 for P2PKH, 5 for P2SH) alongside existing testnet bytes (111, 196)
• Payload length validation (20 bytes) prevents silent corruption from malformed addresses
• Replacing the bare assert c in b58chars with a descriptive ValueError and the assert False catch-all with raise ValueError improves debuggability
• The wit_ver/wit_payload rename is a nice clarity improvement over shadowing version/payload
• Unit tests cover both valid mainnet/testnet conversions and all error paths (empty address, unsupported version byte, wrong payload length, invalid base58 character)

No issues found.

Tested ACK 219daccf3b

Ran the address.py unit tests on Windows (Python 3.12):

$ python3 -m pytest test/functional/test_framework/address.py -v
test_address_to_scriptpubkey_base58 PASSED
test_address_to_scriptpubkey_invalid_base58 PASSED
test_base58encodedecode PASSED
test_bech32_decode PASSED

All 4 tests pass, including the 2 new tests added by this PR. The mainnet version bytes (0 for P2PKH, 5 for P2SH) produce correct scriptPubKeys, and all error paths (empty address, unsupported version, wrong payload length, invalid base58 char) raise the expected ValueErrors.

This change seems to be motivated by expanding unit tests for the functional tests without actual functional test code using it. I don't think that is worthwhile doing with the motivation provided. Can you motivate this better? Otherwise I'll close the PR.

Addressed in b37562c

Thanks for the feedback. I added in-tree functional usage in rpc_scantxoutset.py.

That test already routes string destinations passed to sendtodestination() through address_to_scriptpubkey(), so it already exercises the string-address -> scriptPubKey conversion path in functional test code.

Previously, the string-destination cases in this test only exercised the existing regtest/testnet-style Base58 addresses. The added cases extend that same path to the newly supported mainnet Base58 version bytes (0 for P2PKH and 5 for P2SH).

The follow-up checks use raw(scriptPubKey) descriptors so the verification remains independent of network-specific address parsing.

Tested:

• python3 -m pytest test/functional/test_framework/address.py -v
• python3 build/test/functional/test_runner.py rpc_scantxoutset.py

This latest addition doesn't seem useful to me. The code is already covered adequately. I also don't like that this adds random mainnet addresses that have received prior traffic.

Concept NACK, I think this should be closed.