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

💬 jesterhodl commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827634501)
> I've pushed an update to check more broadly. The new issues (for just Polish) include some false positives around (change/reszta). Though, the others largely apply as far as I can see:

I went through all the listed findings in polish translation and corrected most of them. Some of the findings are actually invalid, eg. "reszta" really means "change".

I'm happy we give it another whirl after Transifex changes go in.

NOTE: I also updated some other labels I stumbled upon which need. In genera
...

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

squashed

💬 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_r2058477569)
Inconsistency seems to have been introduced when we started checking for string-version in 9bab521df895c149579b9e64931405c56b008afb in April of 2012. Not on purpose I would wager.

👍 laanwj approved a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318#pullrequestreview-2791268166)
re-ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c

Changed since last: renamed test to `util_mocktime`, added seconds to `SetMockTime` arg, and faulty test is removed instead of replaced by a comment.

💬 maflcko commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2827719802)
lgtm ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c

💬 laanwj commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#issuecomment-2827725228)
LGTM
```
pruneblockchain height

Deletes block and undo data up to a specified height or timestamp.
Requires `-prune` to be enabled at startup; this action is irreversible.

Arguments:
1. height (numeric, required) The block height to prune up to. May be set to a discrete height, or to a UNIX epoch time
to prune blocks whose block time is at least 2 hours older than the provided timestamp.

Result:
n (numeric) Height of the last block pruned

Examples:
> bitcoin
...

💬 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_r2058523521)
Good that we advertise our listening port for outbound peers! Seems kind of unnecessary to tell inbound peers about the address they report as seeing us as (`CService{node.GetAddrLocal()}`):
https://github.com/bitcoin/bitcoin/blob/458720e5e98c6e9103aea1fdfcd39bafc26c27bb/src/net.cpp#L246-L266
But I guess it simplifies the addrman-logic a little bit, not having to populate/refresh it from the local outbound connections.

💬 maflcko commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827758811)
> I went through all the listed findings in polish translation and corrected most of them. Some of the findings are actually invalid, eg. "reszta" really means "change".


Thanks. I was aware that reszta is correct. It was missing from the context, so I've added it in https://github.com/maflcko/DrahtBot/commit/54e4a219e74dbf8064edbe60c0cc6d741381a928

> I'm happy we give it another whirl after Transifex changes go in.

Yeah, I am happy to run the script again, but I do wonder if there is a bett
...

💬 fanquake commented on pull request "test: Suppress upstream `-Wduplicate-decl-specifier` in bpfcc":
(https://github.com/bitcoin/bitcoin/pull/32336#issuecomment-2827779344)
Backported to 29.x in #32292.

📝 vasild opened a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338)
`CConnman::AlreadyConnectedToAddress()` searches the existent nodes by address or by address-and-port:

```cpp
FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())
```

but:

* if there is a match by just the address, then the address-and-port search will not be evaluated and the whole condition will be `true`
* if the there is no node with the same address, then the second search by address-and-port will not find a match either.

The search by address-and-por
...

💬 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_r2058610798)
Opened https://github.com/bitcoin/bitcoin/pull/32338 with this single change, dropping the `|| FindNode(addr.ToStringAddrPort())` condition.

👍 laanwj approved a pull request: "Crash fix, disconnect numBlocksChanged() signal during shutdown"
(https://github.com/bitcoin-core/gui/pull/864#pullrequestreview-2791496832)
Code review ACK 0ee5376fb302ebb21fb6192ed708800ee8035eb5
Nice solution, also handles the (hypothethical) case where a client model is replaced with a new one.