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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)

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.

cc @jnewbery

Code review ACK c75de5d

and I think the change is generally useful.

I think it makes conceptual sense as well now that base58 is only one of the address formats.

ACK c75de5d according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒

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

ACK c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0 according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi9JQv9GtzM9DRsgO0BnO8JJfq58H6VDVwSvMuM1ZSKO1r7uB4F23/h1YYl0U16
Z3EV2I/YzYetLn2Bj+LwqOcQcJWE3cOqb8VxlRud4fLCjm48kssjos8W4eYA2tzY
IjzKnEOUEoq4dXatYOwmNMfnnoPBWR3RnItmTFqP6TkAQFViV11ltr+NbBzMET8I
w1zhkhOtGvPcxry2pcWeFVzo414DP6NfpPjiwQxYKI9JAzf+DxAIbJYZ2WlWjVRs
docESMvjbHvKmAP7oM2mZq5wJ231d3pRIC8/T+mbODmdclVjdjkUjz6sq2WcAG9x
xjJNoHNUClrP3bLOl5y7q5x+u5r/Z4GfoKT/+T1W8jIssReS8f5OK2QSyYZ6GQDh
9+ra2QB/vwkUU/Hqv2hCNveIqO+zuP86Zrgyeusphy6WM4IJrh7WZ/JZCVnrKvXd
DiKnSO/rSgL4ZrlW4gzcs7cfnhSGfuC29GWtA53aTuzDT5U86dNqM+rr3SGrtGAc
1UlcEIKi
=/Rus
-----END PGP SIGNATURE-----

What was the circular dependency that this was fixing?

Concept NACK. This just seems to be moving code around for no good reason. base58 is very much linked to legacy P2PKH and P2SH addresses, so it makes sense that the code for those things should live in the same module (see segwit_address.py where the bech32 and segwit v0 address code are in the same module). The only thing that imports base58 for any other reason are the bytes_to_wif() and generate_wif_key(). Those absolutely shouldn't be in key.py and I have no idea why they were put in a crypo module that previously had no imports.

@jnewbery The circular dependency was script->key->address->script, and I couldn't get the taproot tests to work without it.

If you have another solution, I'm all ears, but this seems like a generally clean solution to me.

The circular dependency was script->key->address->script

script doesn't import key

Hmm! Perhaps that was fixed then. I wrote this a few months ago, and was annoyed about keeping to rebase it, so I PRed it separately.

I don't think script has ever imported key in master. It does in your branch, but that doesn't seem like a good reason to open a PR against master

Oops, I see.

Still, I think this is generally an improvement.

I'm simply giving you the feedback that I'd give any other contributor, including someone new to the project: moving the code around for no discernible benefit isn't a good use of reviewer resource.

I'm neutral on whether base58 should have its own module. The bech32 code seems to be perfectly happy in segwit_address.py and I don't see this as any different. I do think it's disappointing that his got two ACKs and was merged within 12 hours even though the motivation and commit log were wrong.

@jnewbery You're right about that. If I would have had the facts right, I would not have opened this PR.