test: support mainnet base58 in address_to_scriptpubkey #34770

1. 
   tonykim525 commented at 11:58 am on March 7, 2026: none

   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
2. DrahtBot added the label Tests on Mar 7, 2026
3. 
   DrahtBot commented at 11:59 am on March 7, 2026: 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
   Concept NACK	sedited
   Stale ACK	Bortlesboat

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

4. test: support mainnet base58 in address_to_scriptpubkey and add error-path tests 013059dcc2
5. test: add explicit empty-address error in address helper 219daccf3b
6. tonykim525 force-pushed on Mar 7, 2026
7. 
   Bortlesboat commented at 10:16 pm on March 7, 2026: none

   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.

8. 
   Bortlesboat commented at 10:38 pm on March 7, 2026: none

   Tested ACK 219daccf3b

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

   0$ python3 -m pytest test/functional/test_framework/address.py -v
   1test_address_to_scriptpubkey_base58 PASSED
   2test_address_to_scriptpubkey_invalid_base58 PASSED
   3test_base58encodedecode PASSED
   4test_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.

9. 
   sedited commented at 11:52 am on March 9, 2026: contributor

   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.
10. test: add in-tree mainnet base58 usage in rpc_scantxoutset b37562c95f
11. 
    tonykim525 commented at 1:26 pm on March 9, 2026: none

    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
12. 
    sedited commented at 1:38 pm on March 9, 2026: contributor

    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.

13. fanquake closed this on Mar 9, 2026

    ---

14. tonykim525 deleted the branch on Mar 9, 2026

Contributors
tonykim525 DrahtBot Bortlesboat sedited

Labels
Tests