💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149664984)
utACK 232928b58a82e3f15307deba1ae921ae2960ccc8

Seems fine to fail early here, instead of running into a `util/time` compile failure later on.

💬 maflcko commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1627639850)
```suggestion
for (const auto& ch: reverse_iterate(buff)) {
```

💬 maflcko commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1627643940)
Alternative, if it doesn't compile with `Span`:

```suggestion
for (const auto& ch: reverse_iterate(std::span{buff}) {
```

💬 brunoerg commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2149692078)
Why not just expanding the tests in `wallet_basic.py`?

💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1627668193)
Sorry, i saw your comment earlier, just didn't have time to comment earlier. Thanks, will push soon.

📝 maflcko opened a pull request: "fuzz: Use std::span in FuzzBufferType"
(https://github.com/bitcoin/bitcoin/pull/30229)
The use of `Span` is problematic, because it lacks methods such as `rbegin`, leading to compile failures when used:

```
error: no member named 'rbegin' in 'Span<const unsigned char>'
```

One could fix `Span`, but it seems better to use `std::span`, given that `Span` will be removed anyway in the long term.

💬 maflcko commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1627691575)
Sure no rush. See also https://github.com/bitcoin/bitcoin/pull/30229

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1627725865)
We may still need to deallocate the old buffer in that case.

💬 sipa commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2149920766)
utACK 07f64177a49f1b6b4d486d10cf67fddfa3c995eb

Making the code more readable is a good reason to make this change, but this code was specifically written to prioritize simplicity over performance. If we'd care about performance, it's probably better to use an approach similar to that of the C reference implementation (as it works entirely on the stack, while the current code does several allocations: 1 in `ExpandHRP`, between 1 and 3 in `CreateChecksum`, and 1 inevitable one in `Encode`).

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627796687)
I've added an Assume that `bits` is in range.

🤔 maflcko reviewed a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2098835816)
e7676b6c52672f557f0e2b036d5e877393f1ce46 🎩

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: e7676b6c52672f557f0e2b036d5e877393
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627567196)
nit in 0b92cd2ae4a317be4fe0dfd208017a7d05d1eb2a: Not sure I understand this concept. Is there anything that is not already enforced normally in C++ that would be enforced by this concept?

For example, `static_cast` in the `Impl()` method, which enforces this concept should already check for inheritance:

```
<source>:17:40: error: static_cast from 'Mixin<FRC> *' to 'FRC *', which are not related by inheritance, is not allowed
```

Conversely, if the class wasn't derived from `RandomMixi
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627736240)
unrelated q in 6370a51aac06a430ec53f29fe03f5bbd08371b34: Can you explain where one finds this "copy()" method?

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627554476)
nit in 0b92cd2ae4a317be4fe0dfd208017a7d05d1eb2a:

Looks like this commit is doing several things at once. It may be easier to review if move-only stuff (like moving rand256 and randbytes function bodies to the header file, and removing the explicit template instantiation) was done in a separate prepare commit?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1627806235)
perfect thanks

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627807244)
nit in c099cd2e3c20fc416e3f7fc1da4798bb8042af1c: Could increase the severity to `LogError()` while touching this?

📝 marcofleon opened a pull request: "fuzz: add I2P harness"
(https://github.com/bitcoin/bitcoin/pull/30230)
Addresses https://github.com/bitcoin/bitcoin/issues/28803.

The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.

<details>
<summary>I2P dict</summary>

```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SE
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627821690)
Well in my view, concepts are really a kind of compiler-enforced documentation. Their primary purpose is giving a readable compiler error when template instantiation ought to be (and often, would be) failing anyway. In this case, someone implementing a new RNG but say missing `rand64`, would instead of getting an error in the instantiation of `RandomMixin` get an error message that references `RandomNumberGenerator`, which then explicitly lists what one is required to do to make it work.

🤔 furszy reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2099269310)
ACK 9db2d8b

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1627825496)
> I've made `m_batch` public so that we can just pass the `WalletBatch` but still use the underlying batch object. However I'm not sure if that's something we want to do in the long term, but I think it should be okay.

Yeah.. I'm not happy reading all the the `batch.m_batch` usages neither but np.