💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717443789)
This hex parsing duplication is a bit painful, will try to come up with a deduplicated alternative tomorrow

👍 danielabrozzoni approved a pull request: "fuzz: remove repeated word in note"
(https://github.com/bitcoin/bitcoin/pull/30651#pullrequestreview-2239056410)
ACK 3f05a1068d10ffe0f2859cd20c5fc9bc8efa1c70

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717508021)
If you are touching this, why not just make this `BOOST_CHECK_EQUAL(EncodeBase58(sourcedata), base58string);` and drop all the other noisy changes here?

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717490960)
Changes like adding `auto` instead of the actual type are mostly noise. I don't think there is precedence for merging these. Can you drop them again (here and in other places where it is the sole change on that line)?

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717510487)
This whitespace was intentional such that the escape patterns can easily be compared with one another. Please leave it like it is.

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717508353)
Unneeded whitespace change.

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717518886)
Not sure if this change is really worth it (and the one adding more context to the case above). There is no randomness involved here and the programmer will have to debug anyway if there is a failure. The other changes for printing some context do make sense, since there is randomness involved and the failure case may not be reproduced immediately.

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717557893)
I don't think this change makes sense, since there is already other stuff added to the test data. I would just leave it as is.

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717554488)
Please stick to the symbol naming conventions in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c - specifically use snake_case everywhere.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717569863)
Done, slightly, `vchCiphertext` -> `chCiphertext` (already some precedence for `ch`-prefix). Also changed `vchSalt` -> `salt` in some places.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717575383)
Added 4 of you in b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795, hopefully you don't mind the company. :)

@maflcko let me know if you prefer a different identifier to make GitHub rendering prettier.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568246)
Done.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562892)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717564420)
Now part of b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568070)
Done.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847)
Changing it to a requires-clause as per https://github.com/bitcoin/bitcoin/pull/30377/files#r1717324499 with additional check for null terminator at the end. Don't think someone would do:
```C++
constexpr char my_hex[] = {'f', 'f'};
constexpr std::array<uint8_t, 1> arr = ArrayFromHex(my_hex);
```
when they could just do:
```C++
constexpr std::array<uint8_t, 1> arr = {0xff};
```

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562300)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983 as suggested in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717386520.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717458216)
Clang unfortunately gives me: ``candidate function template not viable: no known conversion from 'const char[65]' to 'ConstevalHexLiteral' for 1st argument``

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717462054)
I think `*FromHex` jives well with `uint256::FromHex()`.. only thing is the latter stores the bytes in reverse order. :face_in_clouds:

Could go for `VectorFromHex`, but there is already some weak precedence in the type `VecDeque`.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717565594)
I'm burned by old MSVC warnings (probably not current) "implicit conversion to bool" so I'd rather not.