💬 willcl-ark commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356601782)
> Which one is the chain needing this change?

Ah I was slightly unclear; this actually applies to "personal forks" (e.g. my own, your own) as well as actual coin-forks. Anywhere where Cirrus runners, which have more space, are not being used.

> So how does this relate to #33480 (remove centos for alpine) . Is this a fix until that PR gets merged ?

I had thought the alpine job would use less space, but I think they are ~ equivalent, so this fix is still needed on that job for runs on for
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3356612313)
`3aa74b5176...22fadecc14`: rebase due to conflicts and pick changes from https://github.com/bitcoin/bitcoin/pull/33454

💬 Crypt-iQ commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-3356659498)
I am (still) curious why the `!already_in_flight && !CanDirectFetch()` conditional was introduced and I wasn't able to find an explanation in https://github.com/bitcoin/bitcoin/pull/8068 (because I don't understand it, I'm not advocating for changing it). The check to see if we've requested the block doesn't distinguish between if we requested it with `MSG_CMPCT_BLOCK` or `MSG_BLOCK` in the GETDATA. It also doesn't tell us if we requested the block from the peer sending the compact block.

Ther
...

💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3356712363)
`73071b0cdb...5b22928a8e`: fix silent merge conflict with a newly added call to `OpenNetworkConnection()` which upset tidy with `/*strDest=*/` vs `/*pszDest=*/`.

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