requesting review from @laanwj and @MarcoFalke

Concept ACK on moving wif to wallet_util, but it was simply not clear to me that base58 is not allowed to be moved into its own module. I think in the past we had more issues due to files being used for too much (main.cpp, util.cpp, ...) than files being used for too little. The question might be stylistic, so I will refrain from having an opinion. I am fine with keeping base58 in a separate module or moving it back.

ACK on moving the wif key functions elsewhere.

I don't understand what the problem is with base58 being its own module, though. It's used by both addresses and wifs, so it doesn't logically belong in an address module imho.

ACK with or without revert commit.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Add Muhash3072 implementation in Python #19105 (Add Muhash3072 implementation in Python by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ACK, checked that the base58 import is removed from key.py. Still no opinion on whether base58 should be its own module, but the diff here has minimal risk of breaking anything. With two ACKs without commit hash (presumably Concept ACKS?), this seems good to merge.

ACK 3a83a01 🍪

git diff e380818~2 e380818 # Empty diff
git show b216b0b --color-moved=dimmed-zebra # Sort only
git show 3a83a01 --color-moved=dimmed-zebra # move-only, but I also checked that the base58 import is removed from key.py

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 3a83a01694160f2e722e1bc90a328bd569b8e109 🍪

git diff e38081846d889fcbbbc6ca4a4d3bca26807fde2f~2 e38081846d889fcbbbc6ca4a4d3bca26807fde2f  # Empty diff
git show b216b0b71f7853d747af8b712fc250c699f3c320 --color-moved=dimmed-zebra  # Sort only
git show 3a83a01694160f2e722e1bc90a328bd569b8e109 --color-moved=dimmed-zebra  # move-only, but I also checked that the base58 import is removed from key.py
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhfigwAhrV4/KY2hjqXWg1UJZEpP3Oz4KWQNfOPfbS0WwtB+VYz0SASnrq/IYW4
wa1oY38GAfKfB87z+++AmTjuwCogIB3/ECoZo8kwDpX+xdT6TaDOK36avpOf9AfN
XJ9rijDz/7mcBgrgiXjhLPtDITLL5WP11lJwznEZnlPuQ2xCCDLsDHOHHR3v3+ev
2v5BA7oSw5Z57UULWFOFQQG+yW4lraMo5QlbCP+qE8eQnMOp0hoFg3E0l/9Wp9sL
4SNp83nxz1TZqQoQ6Fztp6EBaDwnr6MqT7ozd5GcKYNU5QhYCiMq8F04tL9QlH3K
St/2lKJ89vaAtb8wEIX4heRc/nqDW/XZNDZmabTsUaxGUeSuccyLQWFQsV1EeTiY
NEq1EVaVqEyIEcFCo9LC9GEIDMF9OtynEuvZ8UW4oYX29QRJGlLiVMXlwIyC2v2e
s8Qew3steCB8A365aieTTnQ17npDauJiXrziW308U2PeyC+0+SDBHOiQ969M45f3
CI2MxfgZ
=GPz1
-----END PGP SIGNATURE-----

I don't understand what the problem is with base58 being its own module

No problem, but I think ideally base58, bech32 and higher abstract level address functionality could all exist in the same module with no issues. base58 is used almost nowhere apart from addresses (wallet import format is the one exception I can think of) and similarly for bech32. There's already confusion between the different modules (why is encode(), which encodes a segwit address in segwit_addr.py, but program_to_witness(), which encodes a witness program to an address in address.py? Why are ADDRESS_BCRT1_UNSPENDABLE, etc in address.py when they're segwit addresses?)

Generally, as long as we don't have a guideline of how the test framework classes should be structured into modules, I consider the specifics to be of stylistic nature. (Stylistic specifics are completely up to the author of the commit to decide on. Stylistic feedback may be accepted from reviewers, but it is also valid to simply reject stylistic feedback)

I wouldn't mind merging address and segwit_addr into one module or whatever else is decided to be done. Though, as long as there is no guideline, we shouldn't classify changes concerning modularization as right vs. wrong.