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

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