💬 ChillerDragon commented on pull request "wallet: Improve error log color in the console":
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2149504423)
A light theme indeed is smoother on the eye


I still think the softer color is better. And it fits both dark and light themes.

💬 paplorinc commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2149519045)
@josibake, now that the cherry-pick was merged, I've rebased and re-measured - the important part of this change was already included in the other PR, the remaining optimizations here are smaller, but also a lot simpler.

💬 hebasto commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2149533131)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/220.

💬 hebasto commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/30120#issuecomment-2149534731)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/220.

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