💬 brunoerg commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580229771)
Happy to address it if others like it but I personally don't like "approximate" because it's kinda subjective (could mean before or after the height) and do not reflect the goal of it. Also, this issue doesn't happen when it's in IBD.

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2580278793)
> can be simplified to -filter='MerkleRoot'.

Yeah, thanks, I know, I explicitly extended it since it produces two results now.

🚀 fanquake merged a pull request: "Cluster mempool followups"
(https://github.com/bitcoin/bitcoin/pull/33591)

💬 fanquake commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601252308)
@alfonsoromanz can you rebase/ followup with the test failures here?

🚀 fanquake merged a pull request: "[30.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33609)

💬 ajtowns commented on issue "Net split meta issue":
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3601281266)
> Reduce `PeerManager`'s usage of `CNode`
> Move the message handling loop to `PeerManager`

Big fan of these aspects. I like the documentation aspect of NetManagerEvents. Would like to see the PeerManager functions move towards accepting a `Peer&` (vs a `CNode*` and looking up `GetPeerRef`, or an explicit `CNode, Peer` pair) in general too.

> Dropped usage of `CNode` in `PeerManager`
>
> With the event loop moved and nascent interfaces beginning to take shape, `PeerManager` should be able to
...

💬 fanquake commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3601283102)
Can we just pass the compiler through?

💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580569951)
Should be in `compat.h`?

💬 fanquake commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#issuecomment-3601316432)
cc @Sjors

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3601375713)
> if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"

sv2-tp always sets the value (based on `coinbase_output_max_additional_size` in `CoinbaseOutputConstraints`), so it wouldn't benefit. But it does seem better to allow clients to omit this value.

For that to work however, we'd also need to change mining.capnp, e.g.:

```capnp
blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
hasBlockReservedWeight @3 :Bool
...

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2580623784)
The latest push leaves RPC code untouched.

💬 willcl-ark commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3601387596)
> See [#33902 (comment)](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641).

As I describe [here](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2556778530) I have a commit which if `host_CC` is set, **and**** `CC` is unset, will set `CC` to `host_CC` (and similarly for `CXX` and friends).

In my opinion this would result in the expected behaviour:

- If `{var}` is set, use that
- If not check if `host_{var}` is set, and use that
- Otherwise, fallback to a
...

🤔 rkrux reviewed a pull request: "test: p2p: check that peer's announced starting height is remembered"
(https://github.com/bitcoin/bitcoin/pull/33990#pullrequestreview-3529561300)
Concept ~0 52f96cc235d309d9156eb742036c859984b9a467

I'm on the fence regarding whether this case testing just one field is warranted. I don't see much coverage gained in the `corecheck`.
Though there is a similar `test_desirable_service_flags` case as well but in it the connection is disconnected conditionally, so one can argue that there is more to test over there.

💬 brunoerg commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#discussion_r2580745364)
nit: Perhaps a comment explaining these values (edge cases for 32-bit integers) would be good.

💬 brunoerg commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3601527993)
Can be tested with the following diff:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5dac413319..ec417b97ac 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
LOCK(pfrom.m_subver_mutex);
pfrom.cleanSubVer = cleanSubVer;
}
- peer->m_starting_height = starting_height;
+ peer->m_starting_height
...

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601633659)
> @alfonsoromanz can you rebase/ followup with the test failures here?

Yes, I will take care of it

👍 laanwj approved a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3529672889)
Re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db
```
$ git range-diff 3bb30658e631ed45b6c8609292facc7ae3dd0f61..fa2fd0ba1fbd4085df24a2c5472636957db28521 b30262dcaa28c40a0a5072847b7194b3db203160..fad61185861a6a9ed806c387aa63d2b31262b1db
```
⋮
* `4: 4444edeecc528ccd88b7dd78ffddbccd69762132 ! 4: fae612424b3e70acd6011a4459518174463b3424 contrib: Remove confusing and redundant encoding from IO` - the only differing commit, checked that it is rebase

⋮

💬 brunoerg commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#issuecomment-3601640495)
Rebased

👍 stickies-v approved a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3529707358)
ACK e4703863655135856d0d6ad54a7021d3326bf071

💬 stickies-v commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580857428)
"Blocks after target height may be processed during shutdown." is a bit more concise and has my preference.