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

💬 jesterhodl commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827592786)
> #### Erroneous translation:
> ```
> <source>Warning: If you encrypt your wallet and lose your passphrase, you will &lt;b&gt;LOSE ALL OF YOUR BITCOINS&lt;/b&gt;!</source>
> <translation>hasłoOstrzeżenie: Jeśli zaszyfrujesz swój portfel i zgubisz hasło - &lt;b&gt;STRACISZ WSZYSTKIE SWOJE BITCONY&lt;/b&gt;!</translation>
> ```
Transifex Updated

> #### Erroneous translation:
> ```
> <source>Wallet to be encrypted</source>
> <translation>Portfel do zaszyfrowania </translation>
> ``
...

💬 VolodymyrBg commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#discussion_r2058428705)
Done

👍 maflcko approved a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318#pullrequestreview-2791196960)
lgtm

💬 maflcko commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#discussion_r2058447384)
```suggestion
SetMockTime(111s);
```

nit: Could clarify while touching

💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2058445224)
These are the best I could come up with:
* `IsConnectedToStrAddr`
* {`IsConnectedToService` | `IsConnectedToAddrPort`}
* `IsConnectedToAddress`

`Is`-prefix == short way to signify boolean query function.

💬 laanwj commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2827632336)
Would make sense to squash this to one commit imo, no need to introduce the comment just to remove it again.