💬 0xB10C commented on pull request "test: Suppress upstream `-Wduplicate-decl-specifier` in bpfcc":
(https://github.com/bitcoin/bitcoin/pull/32336#issuecomment-2827076150)
Post merge ACK

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2058025067)
Thanks! (If you re-touch: `std::span{key_bytes}`)

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2058041033)
Ah, I was interpreting "Needed for unobfuscated Read" as applying to `Read`-calls *after* the ctor, hadn't realized it was for the call 2 lines below. :man_facepalming: That's why I didn't think it was relevant to include in https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057656754. Sorry for this detour. `assert` is good.

Maybe comment could spell out "Needed for unobfuscated Read *directly below*" for similar readers to me, but hopefully there aren't too many of them.

📝 laanwj opened a pull request: "qt: Replace stray tfm::format to cerr with qWarning"
(https://github.com/bitcoin-core/gui/pull/868)
GUI warnings should go to the log, not to the console (which may not be connected at all).

👍 laanwj approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2790646401)
re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e
git range-diff b2bb27f40c7ab7b51720e41d136dcc9077a525b3..e39118ab0bf08fdaf68b16c86ad304b9133b43c7 9a4c92eb9ac29204df3d826f5ae526ab16b8ad65..17bb63f9f9b08e6af60c089234fe878657dbc88e
- `test: wallet_transactiontime_rescan importdescriptors always rescans`: commit removed (test change not necessary here)
- `test: Remove legacy wallet tests from wallet_backwards_compatibility.py`: comment change typo reverted
- `wallet: Disallow legacy wallet creat
...

💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827238571)
Pushed another update to check for the format spec, but those are quicker and easier to find via:


```
$ git grep -l '1%1' src/qt/locale/
src/qt/locale/bitcoin_bg.ts
src/qt/locale/bitcoin_bn.ts
src/qt/locale/bitcoin_da.ts
src/qt/locale/bitcoin_es.ts
src/qt/locale/bitcoin_es_CL.ts
src/qt/locale/bitcoin_es_CO.ts
src/qt/locale/bitcoin_es_DO.ts
src/qt/locale/bitcoin_es_SV.ts
src/qt/locale/bitcoin_es_VE.ts
src/qt/locale/bitcoin_it.ts
src/qt/locale/bitcoin_ko.ts
src/qt/locale/bitcoin_pl.ts
src/qt/loc
...

🤔 shahsb reviewed a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-2790763507)
Concept ACK

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2058207242)
> I assume addr-gossip only contains address+port from _outbound_ peers ...

The origin of the gossip is the self-announcement done by each node:

https://github.com/bitcoin/bitcoin/blob/458720e5e98c6e9103aea1fdfcd39bafc26c27bb/src/net_processing.cpp#L5286-L5300

then as this address is received by a node, it is further relayed to other nodes. Eventually everybody stores it in their addrman. Then [`GETADDR` requests](https://developer.bitcoin.org/reference/p2p_networking.html#getaddr) are
...

💬 laanwj commented on pull request "gui: crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2827386509)
My preference would be, instead of this fairly fragile check, to symmetrically disconnect the `numBlocksChanged` signal in `setClientModel` when the clientModel is unset.

💬 laanwj commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2827429481)
> The schema could be unwrapped further, or the data could be sent in some binary json format depending on what your goal is but that interface was the simplest way I could think of implementing this feature.

Right, Say, representing the entire RPC protocol as cap'n'proto structures directly would be interesting (anologous to how c-lightning has a gRPC version of their RPC), but this would be a huge project for which the first step would be the formal definition of the RPC protocol (including
...

⚠️ fanquake opened an issue: "test: bip324_tests & net_tests failure with `-O3 -flto`"
(https://github.com/bitcoin/bitcoin/issues/32337)
```bash
x86_64 / aarch64
Alpine 3.21.3
master @ 458720e5e98c6e9103aea1fdfcd39bafc26c27bb
GCC gcc (Alpine 14.2.0) 14.2.0
ld GNU ld (GNU Binutils) 2.43.1
```

```bash
cmake -B build -DAPPEND_CFLAGS="-O3 -flto -fvisibility=hidden" -DAPPEND_CXXFLAGS="-O3 -flto -fvisibility=hidden" -DAPPEND_LDFLAGS="-flto"
cmake --build build
```

```bash
ctest --test-dir build --output-on-failure
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 19: bip324_tests
1/2 Test #
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2058288419)
So this:

<details>
<summary>[patch] rename</summary>

```diff
--- i/src/net.h
+++ w/src/net.h
@@ -1355,24 +1355,24 @@ private:
/**
* Determine whether we're already connected to a given "address:port".
* Note that for inbound connections, the peer is likely using a random outbound
* port on their side, so this will likely not match any inbound connections.
* @param[in] str_addr String of the form "address:port", e.g. "1.2.3.4:8333".
* @return tru
...

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2058295285)
https://github.com/bitcoin/bitcoin/pull/31144/checks?check_run_id=41075184292
Was able to repro with GCC using:
```
cmake -B build -DCMAKE_BUILD_TYPE=Debug -DAPPEND_CPPFLAGS="-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC"
```
We both forgot to limit the span:
```C++
const bool all_zeros{!obfuscation || std::ranges::all_of(std::span{key_bytes}.first(min(write_size, key_bytes.size())), [](auto b) { return b == std::byte{0}; })};
```

💬 laanwj commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2827463844)
Looks like a follow-up to #32212 (first thought it was a duplicate).

💬 VolodymyrBg commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2827469885)
> Looks like a follow-up to #32212 (first thought it was a duplicate).
But issue is opened

💬 VolodymyrBg commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2827478307)
> Looks like a follow-up to #32212 (first thought it was a duplicate).

#32212 was merged 3 weeks ago and the author is maflcko, but he left a comment that this test can be deleted 2 weeks ago

💬 maflcko commented on issue "ci: failures in cross-built Windows CI":
(https://github.com/bitcoin/bitcoin/issues/32197#issuecomment-2827478590)
Looks like the failure did not happen again for the last two weeks, so far?

👍 laanwj approved a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318#pullrequestreview-2791014170)
Code review ACK 51e4c5b42260855e5b5306d677ad34560976b9fd, timing-based tests are brittle, and i don't think this is a very important thing to test as part of our test suite.

💬 maflcko commented on issue "test: bip324_tests & net_tests failure with `-O3 -flto`":
(https://github.com/bitcoin/bitcoin/issues/32337#issuecomment-2827497286)
It would be good to reduce the test case and report it upstream. Though, similar to https://github.com/bitcoin/bitcoin/issues/32276, it seems tedious to reduce.

💬 maflcko commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#discussion_r2058378961)
not sure we need to keep a comment about tests that were removed. What is the goal here? If the goal is to prevent re-introducing the test, it seems weak, because the comment can be missed easily. Also, the remainder of the code here in this function is concerned about mocktime, so it would be better to remove the comment and rename the test name to clarify it is about mocktime only.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2058384454)
but, but, but... :) this function does care to compare the port as it compares `node->m_addr_name` to `addr.ToStringAddrPort()`. If the argument is changed from `CAddress` to `CNetAddr`, then it would need another argument - the string "addr:port". This oddness is the same in `master`. Now I wonder if this condition:

```cpp
node->addr == net_addr || node->m_addr_name == str_addr

// or in master, which is the same:
// FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort
...