📝 achow101 opened a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658)
Update chainparams and headerssync parameters for the pre-28.x branching, per https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220)
New version of the comment in latest push:
```
* @warning Unlike VecFromHex which returns a vector, the returned array may use
* significant stack space if called inside a function.
```

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2238863495)
Code review ACK 65bc6dcd573fe74d2ce175466c30ed830d17f0fc. Looks good, thanks for working on this, and I think this will avoid the need to add the ParseHex suppression in #30415. Left a few comments but nothing critical.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717352300)
In commit "test refactor: util_tests - Use BOOST_CHECK_EQUAL_COLLECTIONS()" (c6e1d5bff44b7d382a5a91b6722743b5fe07a95d)

Might be worth mentioning this change in commit message or saying it has other test cleanups.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717386520)
In commit "refactor: Hand-replace some ParseHex -> ArrayFromHex + VecFromHex" (8ad7e7d3fdfed4b126859dd83ab81cf8a8ae82a9)

It's not obvious why an extra ToByteVector conversion is necessary here and why ArrayFromHex is not sufficient. Does CScript not accept std::array, or does it not accept uint8_t? I think this change is ok but it would be good to say in commit message what is happening here and how we could simplify this in the future, assuming it is something we should be able to clean up l
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717373651)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b997d266e549ad6745d01a696cd9fbf1d3525944)

It seems like this is assuming the last character in the string is \0 and then ignoring it? I guess that is good because it makes this convenient to call, but in theory it means somebody could call this with a character array that is does not end in \0 and then the last character of the string would be ignored.

Would suggest changing this to:

```c++
size_t size{hex_str[Size-1] ==
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717392937)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220

> New version of the comment in latest push:

Thank you! That seems accurate now.

🤔 stickies-v reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2238242329)
Approach ACK

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