💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2149356751)
ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba 🔳

<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: ACK 2431ecff2fb6f4b68d3de0ea36
...

📝 fanquake opened a pull request: "build: no-longer allow GCC-10 in C++20 check"
(https://github.com/bitcoin/bitcoin/pull/30228)
Reverts part of fa67f096bdea9db59dd20c470c9e32f3dac5be94, now that we require a minimum of GCC 11.

See also:
https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1745143612.

💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149383116)
Is there any user facing difference? If so, it would be good to show it in a comment.

💬 fanquake commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149392998)
> Is there any user facing difference? If so, it would be good to show it in a comment.
```bash
./configure CC=gcc-10 CXX=g++-10
<snip>
configure: error: *** A compiler with support for C++20 language features is required.
```

💬 maflcko commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149410956)
Otherwise it would continue to compile (and miscompile `RenameOver`)?

📝 ChillerDragon opened a pull request: "wallet: Improve error log color in the console"
(https://github.com/bitcoin-core/gui/pull/823)
# before



# after


💬 fanquake commented on pull request "build: no-longer allow GCC-10 in C++20 check":
(https://github.com/bitcoin/bitcoin/pull/30228#issuecomment-2149420217)
> Otherwise it would continue to compile (and miscompile RenameOver)?

I don't know where it would fail first, but I don't see the point of having it continue past configure in this case.
There are multiple devs using clang-14 who've been confused by clang-14 failing to compile code, and unsure what the issue was. So fixing the case here for GCC seems more useful than random compile errors.

🤔 MarnixCroes reviewed a pull request: "wallet: Improve error log color in the console"
(https://github.com/bitcoin-core/gui/pull/823#pullrequestreview-2098729890)
The default GUI is in light-mode, `red` looks fine there for me.
Other warning messages throughout the GUI are also using `red`, they should all use the same color.

🚀 fanquake merged a pull request: "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure"
(https://github.com/bitcoin/bitcoin/pull/30174)

🚀 fanquake merged a pull request: "guix: show `*_FLAGS` variables in pre-build output"
(https://github.com/bitcoin/bitcoin/pull/30185)

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