👍 l0rinc approved a pull request: "headerssync: Preempt unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2954075447)
I find the current layout of tiny, focused commits a lot easier to follow, thanks!
Left a few comments, after that I will test it again locally and I can ACK.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164197118)
this comment is obvious from the code now: `/*exp_locator_hash=*/Params().GenesisBlock().GetHash()` or `/*exp_locator_hash=*/genesis_hash` later in 65a96b507c11b3f34efa996919892fa1a2fcf49c

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164220997)
is this comment accurate?

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164235547)
```suggestion
// 5. Sets the redownload buffer size to be large enough that we
```

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164194785)
would't an `.at(0)` suffice?

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164239931)
nice, this is exactly how comments should be used to augment what the code cannot easily tell <3

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164212315)
I'm not sure what "this many headers *on top* have been received"

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164149745)
ah, so it's jut a reassurance that it doesn't need a fancy asic, it can mine a block in no time - makes sense

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164245384)
might make sense to extract genesis hash here as well like we did in `HappyPath`

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164207447)
> makes it more likely that the number changes between releases

That sounds like a counter-argument to me, but don't have strong feelings about it, if you disagree, I don't mind, please resolve it.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164253700)
Can we add 2 helpers in that case? I really dislike that the comments don't follow the code's structure :.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164260873)
> chunks of code to minimize diffs

We've rewritten the tests at this stage, diffs and the function prototypes don't help in my opinion.
But if you insist, I don't mind, please resolve the comment

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2164267943)
Not sure I understand, especially in the new code which states something like `locator == genesis` - which is basically the same as the comment

💬 polespinasa commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000887231)
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d

Nothing to add to the comments already done.

🤔 janb84 reviewed a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2954293190)
reACK e27a94596f2a1f5e04722a16165717cc6e891d36

Changes sinds last ACK:
- CMAKE_SYSTEM_PREFIX_PATH -> CMAKE_PREFIX_PATH

Validated that change still solve problem ✅

📝 Prabhat1308 opened a pull request: "test: add functional test for upgradewallet rpc"
(https://github.com/bitcoin/bitcoin/pull/32803)
followups up from https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2875411238 to add coverage for `upgradewallet` rpc. PR adds 2 test cases , trying to upgrade from the latest version and asserting it stays the same and trying to downgrade from the latest version. Test case to actually upgrade is not possible currently since the `createwallet` rpc by default creates wallet with version `FEATURE_LATEST` which is the highest.

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164322315)
ok taken this, thanks.

new constructor is

```cpp
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```

so its like "passed in an `any` which i convert to a `NodeContext` pointer which I assert is not null, then de-reference back to a `NodeContext` and store that as a reference in the class

💬 ryanofsky commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000963484)
The reason the `ipcbind` option doesn't support TCP currently is that there's no authentication, so listening on a TCP socket would let any process on the system running as any user connect to the port, and potentially access wallets or take advantage of privileges the bitcoin process has.

So currently:

- ssh provides a reasonable way to forward sockets with permissions and authentication (`ssh -L /tmp/local.sock:/path/to/remote.sock user@remotehost` or `ssh -R /tmp/remote.sock:/path/to/local.
...

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164325336)
Good idea, but would that reduce confidence in the test coverage for `RPCRunLater()` etc?

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2164329006)
See https://github.com/asmap/asmap-data/issues/25

The repository description there says:

> This repository is strictly used for demonstration purposes to help with conceptual discussions about ASMap in the Bitcoin Core release process. Any data uploaded here should be treated with extreme caution and is not inteded for production use.

which doesn't seem to match the expectations around the repository as used here.