💬 josibake commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#discussion_r1627539860)
in https://github.com/bitcoin/bitcoin/pull/29607/commits/e7c55d4f4dea4c42b40ff326a17e6884de181ed6:

Is there a reason to prefer `auto` over explicitly mentioning the types? I find it to be more readable as

```suggestion
for (const char& c : hrp) assert(c < 'A' || c > 'Z');

std::string ret;
ret.reserve(hrp.size() + 1 + values.size() + CHECKSUM_SIZE);
ret += hrp;
ret += '1';
for (const uint8_t& i : values) ret += CHARSET[i];
for (const uint8_t& i : Create
...

💬 paplorinc commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#discussion_r1627559640)
Done, thanks

💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149598517)
Thanks for checking. Can you re-try on 27.x, because https://github.com/bitcoin/bitcoin/commit/c3530254c926a5ab9b710512bbbd29e5cd5b10f7 is not in 27.x.

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

Verified the benchmark locally. Aside from the speed improvements, this also improves the readability of the code.

👍 shaavan approved a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2098882561)
ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba 🚀

💬 fanquake commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149620807)
> Can you re-try on 27.x, because https://github.com/bitcoin/bitcoin/commit/c3530254c926a5ab9b710512bbbd29e5cd5b10f7 is not in 27.x.

27.x & fccd32efe6e2950b2c74fdec2ade54040ca32a2c compiles with `gcc-10 (Ubuntu 10.5.0-4ubuntu2) 10.5.0`.

💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149663564)
Oh, 27.x remains on g++-10, so no backport needed. I was wrong. See https://github.com/bitcoin/bitcoin/blob/27.x/doc/dependencies.md

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