🤔 l0rinc requested changes to a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-3285962003)
Rebased locally and did a first pass through the changes.
It seems to me the `capnp` wrapper needs some updates (missing field, 32/64 bit params, param rename, bool vs uint64, enums as unsigned, optional mapping, unused declarations, comments, etc).

Unrelated nit: while checking the code I noticed that `RemoveCvRef` and `std::remove_cv_t<std::remove_reference_t` can likely be changed to `std::remove_cvref_t` in a few places (should be done in a different PR, if valid)

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2392292721)
Could we add a simple explanation comment for these salts?

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559741869)
Original has enums for `reason`
https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L323

should we make the param `UInt32` like we did one line below for `ChainstateRole`
```suggestion
transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :UInt32) -> ();
```

Or vice versa, since `Int32` seems more common (though `UInt32` may make more sense).

But looking at https://github.com/bitcoin/bitcoin/pull/29409#discussion_r18
...

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2553243415)
super-nit: I assume the copyright headers are similar to other code in the repo:
```suggestion
# Copyright (c) 2024-present The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://opensource.org/license/mit.
```

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2392340952)
I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id?

https://capnproto.org/language.html#evolving-your-protocol claims:
* New fields, enumerants, and methods may be added to structs, enums, and interfaces, respectively, as long as each new member’s number is larger than all previous members. Similarly, new fields may be added to existi
...

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559735548)
May be deliberate, but the parameters in
https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L267
are 32 bit

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559789059)
If this corresponds to
https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/rpc/server.h#L50
we might want to ment why this one doesn't have a corresponding proxy:
```suggestion
# transport-only struct corresponding to `std::pair<std::string, bool>` in `CRPCCommand` constructor
struct RPCArg {
```

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559765741)
The original param name is `delta_seconds` (where it doesn't make sense, since it's typed), but here it would, since it's not obvious what `Int64` should contain otherwise (could be epoch millis as well as far as I can tell).

https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L426

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559791820)
same, should likely be

```suggestion
mode @3 :Int32;
```

💬 l0rinc commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559797683)
Is this still used after https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3096459508?

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-3575743451)
Is this ready for review now?

👍 fanquake approved a pull request: "ci: Remove redundant busybox option"
(https://github.com/bitcoin/bitcoin/pull/33903#pullrequestreview-3505228993)
ACK fa0fee44a89c82750a39e9d54bb5a6fc72b77fce

💬 Sjors commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3575819797)
In terms of interface changes I'd like to focus on #33819 `getCoinbase()` and #33922 `getMemoryLoad()` which add new functionality. The smaller breaking interface changes can wait until either we have a bigger breaking change or nothing else on our plate.

On the IPC side I don't really have priorities in mind, so maybe pick what's useful for @plebhash (as a stand-in for others who are both unfamiliar with the Bitcoin Core codebase and using other programming languages to connect).

💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3575872453)
@m3dwards

> Not necessarily an issue with this PR but when trying to test on Trixie with `make -C depends HOST=x86_64-w64-mingw32ucrt -j9`
>
> I get: ...

Interesting.

I’m unable to reproduce this in a fresh Trixie container using Docker:
```
# apt update
# apt install git cmake curl make patch bison g++ ninja-build pkgconf python3 xz-utils g++-mingw-w64-ucrt64
# git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin
# gmake -C depends -j 16 HOST=x86_64-w64-mingw32ucrt

...

💬 plebhash commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3575893233)
we're doing a SRI release this week that is planned to include https://github.com/stratum-mining/sv2-apps/pull/59, and it's meant to support v30

but it will come with two major limitations, namely:
- the initial iteration of `bitcoin_core_sv2` crate is marked with a bunch of `todo`s that are workarounds for the lack of `interruptWait`, and are being tracked at https://github.com/stratum-mining/sv2-apps/issues/81
- running it on systems with low RAM for extended periods of time can lead to crash
...

✅ hebasto closed a pull request: "test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable"
(https://github.com/bitcoin/bitcoin/pull/32409)

📝 hebasto reopened a pull request: "test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable"
(https://github.com/bitcoin/bitcoin/pull/32409)
This PR adds a way to suppress the abort message box when running `test_bitcoin.exe` and `fuzz.exe` built with the debug runtime library on Windows.

Otherwise, a failing `assert()` triggers the `abort` routine, which displays a message box and causes a timeout in CI.

Here are CI jobs for the "Debug" configuration:
- on the master branch: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/14759346479/job/41435857332
- with this PR: https://github.com/hebasto/bitcoin-core-night
...

💬 yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3575923284)
Thanks for the review @TheCharlatan.
- Addressed [comment](https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554084325) on using post-condition checks.
- Dropped the change to use separate state for reporting errors and not invalidity as suggested by @TheCharlatan [comment](https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554092568)

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3576053570)
Rebased after https://github.com/bitcoin/bitcoin/commit/4f65a1c5db84426b9d814a18f63ac46632691ab2#diff-f14e9423bd43609969886b2e42751c55606344e0b71780564cdb23515802cec0R12, ready for review again.

🤔 rkrux reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-3505373212)
Concept ACK 6e523bda47854333d267f53cfa5292fd2eb40ec7

Agree with the intent to avoid rescanning if the user specifies so.