🤔 jonatack reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2554013096)
re-ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3

🤔 darosior reviewed a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2553493851)
Some more nits i noticed when reusing some of this code for the fuzz target. I don't think they warrant retouching, but figured i'd post them anyways.

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917096066)
nit in the context of this test but i think you meant:
```suggestion
if (service.SetSockAddr(sa, sa_len)) {
// Can only bind to one of our local ips
if (!service.IsBindAny() && service != m_local_ip) {
return -1;
}
m_bound = service;
return 0;
}
return -1;
```

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917096949)
nit
```suggestion
assert(false && "Move of Sock into PCPTestSock not allowed.");
```

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917105856)
nit: it's never used, any reason for defining it?

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917273199)
Isn't this redundant with `~Sock`?

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1917172067)
nit
```suggestion
0x02, 0x81, 0x00, 0x42, // version, opcode, result error
```

👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2553957182)
Code review ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361. Main change since last review is mining_testnet.py test being replaced by mining_mainnet.py test with a much smaller data file. It was also rebased and there were small documentation changes and string format tweak in mining_mainnet.py

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1917334042)
In commit "Use OP_0 for BIP34 padding in signet and tests" (75342f7e19cbbae083c2a3561f2c2cefd4e2968a)

Do we know any reason why this code was using OP_1 instead of OP_0 previously? Any possible advantages to using OP_1? It looks like this code was introduced in #16363

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1917359516)
In commit "test: check difficulty adjustment using alternate mainnet" (a70f967bda5a0998a2b5a851cd32d969b021df15)

This seems ok, but I think I don't understand why it is useful to test that coins are spendable. It seems like something besides the point of the test (at least as stated in the description), which is to test difficulty retargeting functions on the main chain that can't be tested on the regtest chain. I wonder if this is spending test is checking something that existing tests are n
...

📝 ismaelsadeeq opened a pull request: "Fees: add Fee rate Forecaster Manager"
(https://github.com/bitcoin/bitcoin/pull/31664)
- This PR implements the core component of #30392, introducing a new fee rate forecasting module.
The primary addition is a `ForecasterManager` that coordinates multiple forecasting strategies to be able to provide better transaction fee rate predictions.


### Key Changes

1. **Addition of Fee Rate Forecasting Utility Structures**
- `Forecaster` abstract class, serving as the base class for all fee rate forecasters.
- `ForecastResult` (the response from a `Forecaster`) with a
...

🤔 mzumsande reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2554067385)
ACK 86d7135e36efd39781cf4c969011df99f0cbb69d

I reviewed the code and tested it a bit on mainnet with some extra logging: The large majority of orphans gets resolved, a few orphans get stuck for 20 minutes if all of our candidates send `NOTFOUND`s for the parent requests, presumably because the parent got removed from their mempools (this has been mentioned in https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2523122642 ).

💬 jonatack commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2594008381)
Concept ACK

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1917448533)
Good idea, thanks. I've removed the implicit check.

📝 davidgumberg opened a pull request: "build: Make config warnings fatal if -DWERROR=ON"
(https://github.com/bitcoin/bitcoin/pull/31665)
While reviewing #31593, which upgrades the CentOS version in CI from Stream 9 to Stream 10, I discovered that the CentOS ci task had been attempting to configure a build with python `3.9` (the latest version in the Stream 9 core repo) below the minimum version of `3.10` which resulted in a warning being appended to the `configure_warnings` cmake/env variable. https://github.com/bitcoin/bitcoin/blob/712cab3a8f8ad76db959337ddc35cb4c34cac388/CMakeLists.txt#L546-L553

This warning has been emitted
...

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2594120423)
Updated commit message since this commit now also removes an assertion as discussed in this PR review.

💬 davidgumberg commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2594133167)
Upstream tracking issue for branching and adding `dash` into EPEL 10 is here: https://bugzilla.redhat.com/show_bug.cgi?id=2335416

📝 glozow opened a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666)
Followup to #31397. Draft until merged.

Addressing (in order):
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906077380
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1881060842
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905994963
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905999581
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906001592
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905989913
https://githu
...

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917490318)
Thanks, done.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917491100)
How to best make this compatible with podman and/or macos remains an open question, but I've switched to using `CI_IMAGE_PLATFORM` that gets passed to `docker build` and `docker run` as you suggested