Remove unsafe uint256S() and test-only uint160S()

[stickies-v]

Continuation of #30569.

Since https://github.com/bitcoin/bitcoin/commit/fad2991ba073de0bd1f12e42bf0fbaca4a265508, uint256S() has been deprecated because it is less robust than the base_blob::FromHex() introduced in https://github.com/bitcoin/bitcoin/pull/30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also https://github.com/bitcoin/bitcoin/pull/30532)

This PR removes uint256S() (and uint160S()) completely, with no non-test behaviour change.

Specifically, the main changes in this PR are:

• the (minimal) last non-test usage of uint256S() in ParseHashV() is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying this test commit).
• the test usage of uint{160,256}S() is removed, largely replacing it with uint{160,256}::FromHex() where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant
• the now unused uint{160,256}S() functions are removed completely.
• unit test coverage on converting uint256 <-> arith_uint256 through UintToArith256() and ArithToUint256() is beefed up, and arith_uint256 tests are moved to arith_uint256_tests.cpp, removing the uint256_tests.cpp dependency on uint256h, mirroring how the code is structured.

Note: uint256::FromUserHex() exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to uint256S() where necessary.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, hodlinator, ryanofsky
Concept ACK	maflcko

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

[stickies-v]

Force-pushed to add commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to remove the test-only arith_uint256S() function entirely. Since arith_uint256 does not have any string string constructors, it uses uint256 constructors, and those are already unit tested. In some cases, a string constructor can be avoid entirely, e.g. by using the arith_uint256 uint64_t constructor.

Addresses @l0rinc's review comment about unnecessary tests and unnecessary docstring.

[stickies-v]

And force-pushed again to rebase on top of #30377 to avoid silent merge conflicts e.g. because of the now lowercase-only uint256 hex string constructor.

[l0rinc]

reACK 9ef049db5ea30f60d0b72714c42c74e2d816c820

[stickies-v]

Force-pushed to remove behaviour change in commit "rpc: use uint256::FromHex for ParseHashV", addressing @maflcko's concerns over behaviour change. I don't think this is necessarily an improvement, but I appreciate that's my subjective view and the (slight) behaviour change was orthogonal the goal of the PR anyway so it seems like the most pragmatic approach to make progress. Thanks for your review!

[l0rinc]

ACK da4377dc2ad0f495ebb97722d6cc2a95850363fa

I didn't mind the previous version either, but maybe this preserves more of what's not strictly related to the PR. I think it's an improvement as-is, but I will gladly reack if you decide to include the other reviews as well.

[stickies-v]

Force-pushed to increase uint256/arith_uint256 conversion tests (hopefully) addressing @maflcko's concern, (or at the very least just improving test coverage). To avoid changing the same lines multiple times, I've squashed the conversion move commit into 6cfa7f4a0361d9c396d1c5bd71849295baf6290d.

Also adopted two style nits on happy-path and limiting variable scope.