💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499)
nit: The size requirement could also be expressed as a requires clause:
```cpp
consteval std::array<Byte, (Size - 1) / 2> ArrayFromHex(const char (&hex_str)[Size])
requires (Size % 2 == 1)
```

It provides more helpful information on why the constraints aren't met, e.g.:

```
net_processing.cpp:3934:39: error: no matching function for call to 'ArrayFromHex'
constexpr auto finalAlert{ArrayFromHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f000000
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716969813)
nit: `vch` var name prefixes are annoying now that they're not vectors anymore. It'll increase the diff but I still think it might be worth modernizing the names here to avoid that inconsistency? (here + a bunch of other instances)

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717177489)
nit: calling this non-iterator `i` would be more clear

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717088367)
Testing `consteval` functions is quite constrained, but I think it still might be useful to add some happy path test cases to `util_tests.cpp`?

```cpp
BOOST_AUTO_TEST_CASE(consteval_hex_digit)
{
BOOST_CHECK_EQUAL(ConstevalHexDigit('0'), 0);
BOOST_CHECK_EQUAL(ConstevalHexDigit('9'), 9);
BOOST_CHECK_EQUAL(ConstevalHexDigit('a'), 0xa);
BOOST_CHECK_EQUAL(ConstevalHexDigit('f'), 0xf);
}
```

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717416131)
In the PR summary you mean?

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717421534)
By testing these transitively, we wouldn't test everything everywhere. But the latter can have the advantage of making mistakes *really* obvious :)

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717422784)
Yeah :))

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717423143)
I'll take what I can get

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717426365)
Do we really need to support both?

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717427675)
Seems related to: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716886175

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717433775)
Since it's a `consteval` ctor, I _think_ taking a `const char*` parameter should behave the same but avoid having to instantiate for each diferently sized `hex_str` input? But I'm still easily confused by how `consteval` behaves.

```suggestion
consteval ConstevalHexLiteral(const char* hex_str) : inner{hex_str} { Validate(inner); }
```

📝 furszy opened a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659)
Coming from #29073.

Applied @ryanofsky suggested changes on https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 with few modifications coming from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348.

The only point I did not tackle from https://github.com/bitcoin/bitcoin/pull/18338#issuecomment-605060348 is:

> * Move log print and flush out of ReleaseWallet into CWallet destructor

Because it would mean every `CWallet` object would flush data to di
...

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