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

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

🤔 furszy reviewed a pull request: "[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks"
(https://github.com/bitcoin/bitcoin/pull/30385#pullrequestreview-2239237401)
Thanks for the comment gmaxwell.

> I think if you find yourself thinking sending a not found is a solution to a problem, you've got a mistake somewhere else.

Yeah, thats actually how this working path started #28120.

> I believed the current behavior was this: If a node is non-pruned it will have all blocks, no issue. If a node is pruned it will respond to requests for the last 288 which are protocol required and will disconnect any peer that asks for a block outside of the window. Agai
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717580883)
Further changed to just:
```C++
constexpr explicit XOnlyPubKey(Span<const unsigned char> bytes) : m_keydata{bytes} {}
```
Since `base_blob(Span<const unsigned char>)` does the exact length assert internally.