💬 sipa commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3575447242)
Rebased after merge of #33629.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3575453927)
Rebased after merge of #33629.

💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#issuecomment-3575461465)
Needs rebase!

💬 psam21 commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575469910)
@ryanofsky Thanks for pushing for the detailed the investigation. You're absolutely right - I tested it and the `config_env` approach doesn't actually change the behavior.

I compared both scenarios:
Without the fix (master): `env CC="gcc" CXX="g++" ... cmake`
With the fix (this PR): `env CC="gcc" CXX="g++" ... cmake`

Even when running `make CC=clang CXX=clang++`, both produce identical results using gcc. The issue is that `$(build_CC)` defaults to `gcc` in depends/builders/default.mk, a
...

✅ maflcko closed a pull request: "depends: Fix native_capnp to respect build_CC and build_CXX"
(https://github.com/bitcoin/bitcoin/pull/33937)

💬 maflcko commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575480041)
Closing as an LLM generated "AI agent" patch. Please note that contributors are required to fully understand their authored code themselves.

Also, the explanation is obviously wrong and hallucinated.

If you wish to contribute in the future, please focus on creating high-quality, original content that demonstrates a clear understanding of the project's requirements and goals. Also, see the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring).
...

💬 l0rinc commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3575488229)
reACK ade0397f59f2fb59ab0e4ebb39869ac343cc54ee

Redid the rebase (no conflicts, how come rebase was needed?) and soft reset against PR, no changed.
Verifired the same with `git range-diff aef40b93cf057d2a27d61881b0858d491206bcd3...ade0397f59f2fb59ab0e4ebb39869ac343cc54ee`

✅ fjahr closed a pull request: "init: Split file path handling out of -asmap option"
(https://github.com/bitcoin/bitcoin/pull/33631)

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3575555047)
Seems like there is now more opposition to this approach, so I will close this and consider an alternative as a follow-up when the embedding is merged.

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2559959760)
Ah, I get it now, I guess it was late yesterdays. Seems like @fanquake already fixed it.

👍 fanquake approved a pull request: "scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT]"
(https://github.com/bitcoin/bitcoin/pull/29641#pullrequestreview-3505073521)
ACK 317b71bcd2e552f93ac2131b16a8acab414567dd - I think the conflicts here are small, and a number are themselves draft / WIP.

🤔 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;
```