💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149476756)
I am asking, because this may need backport, if compilation succeeds. I assumed it would fail, which is why I left it as-is. However, if this compiles for users using g++10, then it seems dangerous. It would expose them to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98985 and possibly other bugs.

💬 fanquake commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149496033)
The first compile failure I see is in `util/time.cpp`:
```bash
util/time.cpp: In function ‘std::string FormatISO8601DateTime(int64_t)’:
util/time.cpp:50:24: error: ‘year_month_day’ in namespace ‘std::chrono’ does not name a type
50 | const std::chrono::year_month_day ymd{days};
| ^~~~~~~~~~~~~~
util/time.cpp:51:24: error: ‘hh_mm_ss’ in namespace ‘std::chrono’ does not name a type
51 | const std::chrono::hh_mm_ss hms{secs - days};
|
...

💬 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