💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-3356756655)
`c9ffb6d710...87e7f37918`: do https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2394630697

💬 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_r2394864828)
Done.

💬 sipa commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2394877881)
It occurs to me that it's possible the fork point between the active tip and `pindexBestKnownBlock` is ahead of `pindexLastCommonBlock`, but not quite equal to the active tip itself.

Would something like this, unconditionally, not be more general even?

```c++
auto fork_point = LastCommonAncestor(state->pindexBestKnownBlock, m_chainman.ActiveTip());
if (fork_point->nChainWork > state->pindexLastCommonBlock->nChainWork) {
state->pindexLastCommonBlock = fork_point;
}
```

If perfor
...

🤔 janb84 reviewed a pull request: "Clear out space on centos job"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3289659139)
ACK 014b9f6268390b78e66e1be24813d3f069588442

PR introduces new CI action. This action will remove unnecessary packages to reduce space. This action will only run on `GHA` runners and will save about 22gb.

Kinda surprised about the space savings or why are the packages are there in the first place. Anyhow this will help to resolve the GHA space problems for Centos job. (resolves #33293)

🤔 furszy reviewed a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-3289688389)
Code review ACK 87e7f37918d42c28033e9f684db52f94eeed617b

💬 ryanofsky commented on pull request "init: Signal m_tip_block_cv on Ctrl-C":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3356907361)
I'm confused by windows CI failures: [1](https://github.com/bitcoin/bitcoin/actions/runs/18140884308/job/51631167802?pr=33511) [2](https://github.com/bitcoin/bitcoin/actions/runs/18140884308/job/51631960886?pr=33511)

They seem to show py -3 test_runner.py being called, and this just with failing with no output after around 10 minutes, with CI reporting `Process completed with exit code -1073741510.` where -1073741510 is 0xc000013a or [STATUS_CONTROL_C_EXIT](https://learn.microsoft.com/en-us/o
...

💬 l0rinc commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2395019568)
This is meant to avoid a partial initialization, `.value()` throws if we forget to call `FillShortTxIDSelector` (while previously we would have just salted with 0)

💬 sipa commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2395021434)
I was wrong; it's initialized in `CBlockHeaderAndShortTxIDs::FillShortTxIDSelector`.

If we want runtime checks, I'd rather have an `Assert()` or `Assume()` than relying on the throwing behavior of `std::optional<T>::value()`, but that's just a nit.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2395072031)
We can match `JSON` and `JSON_OR_STRING` because both are added to `cmds` list and follow the same rule, and `STRING` is added to string_params list. Recent changes simplify this.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2395073330)
Agreed. Now fixed.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2395075138)
Thanks. Done.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2395078470)
Since the definition of both the `FromPosition` and `FromName` functions tells that they're returning the pointer to the CRPCConvertParam class, thus the parameter names here represent the true meaning in their names according to me. And here `p.rpcMethod == method` if we change `methodName` to `rpcMethod` we need to change the existing name of the member of the class.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2395100057)
As we are not obtaining some value from the iterator the current implementation seems simpler.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3357077663)
`22fadecc14...b01d6df464`: resolve silent merge conflict / fix CI

💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2395186828)
You're not doing that with this.

💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2395187920)
Right, not since the reject message was removed, oh well. :(

💬 l0rinc commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2395190917)
You mean
```C++
return (*Assert(m_hasher))(wtxid.ToUint256()) & 0xffffffffffffL;
```
would be more explicit than
```C++
return m_hasher.value()(wtxid.ToUint256()) & 0xffffffffffffL;
```
?

💬 tyuxar commented on issue "Higher **reported** memory usage of Bitcoin Core after version 29":
(https://github.com/bitcoin/bitcoin/issues/33351#issuecomment-3357203898)
It is not just a reporting issue, there is a problem with the memory use of v29.

I've been running Core in a FreeBSD jail for quite a long time (it's at 14.3p4 right now) and has been using the binary pkg.

v28 used at most approximately 1-1.5 GB, but usually it was hovering around 6-800 MB RSS.

v29 went up to slightly over 3GB shortly after startup, and remained there for 24+ hours. This is RSS! The virtual size was 12+ GB (I assume b/c of the mentioned LevelDB memory mapping).

As I'm runnin
...

💬 cedwies commented on pull request "Avoid file overwriting in fallback `AllocateFileRange` implementation":
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2395335416)
I would be a bit hesitant about mixing `lseek` with `FILE*`, since `stdio` has its own buffering. From what I read, a safer option might be to stick with the `stdio` layer (`fseeko`/`ftello`) or call `fstat` on the `fd` after flushing. Even though I expect mixing the two layers to work most of the time, it can be risky because stdio may have buffered data or be tracking the file position separately from the kernel. I think it's safer to either stick to _stdio_ **or** _fd-only.

💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2395336697)
how so? Care to explain? :)