💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717438506)
leftover?
```suggestion
const CScript testnet4_genesis_script = CScript() << VecFromHex("000000000000000000000000000000000000000000000000000000000000000000") << OP_CHECKSIG;
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717447789)
super-nit (feel free to ignore): we might want to do the content checking after the size check, it's cheaper

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717452225)
```suggestion
// Size should be odd due to implicit null terminator.
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717449874)
super-nit:
```suggestion
if (hex_str.size() % 2) throw "2 hex digits required per byte";
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717446097)
Nit: "vec" vs "array" (abbreviation vs full) -> can we either name them "VecFromHex" and "ArrFromHex" or rather "VectorFromHex" and "ArrayFromHex" (or even shorter: "HexToVector" and "HexToArray"?

💬 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.