💬 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.

⚠️ plebhash opened an issue: "Mining IPC `createNewBlock` should not return before IBD is over"
(https://github.com/bitcoin/bitcoin/issues/33994)
### Please describe the feature you'd like to see added.

Mining IPC `createNewBlock` should not return before IBD is over

### Is your feature related to a problem, if so please describe it.

on SRI [`bitcoin_core_sv2`](https://github.com/stratum-mining/sv2-apps/tree/main/bitcoin-core-sv2) crate, we want to wait for IBD is over as part of the bootstrapping processes

discussing with @ismaelsadeeq and @Shourya742 during BTrust dev day, we came to the conclusion that `createNewBlock` shouldn't ev
...

💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580917867)
Thanks! Reworked.

🤔 alexanderwiederin reviewed a pull request: "kernel: Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891#pullrequestreview-3529821349)
ACK https://github.com/bitcoin/bitcoin/pull/33891/commits/17d3575cfcb41adfd364e7a1912a4701786f7455

💬 alexanderwiederin commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2580952377)
nit: wonder if "efficient" here could be misleading.

💬 fanquake commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3601824876)
Note that 0972f5504021b482b27523fd3bcb8036cf6b439c has broken our `gen-manpages.py` script, by appending additional text (for some bins) to the line containing the version info.

💬 stickies-v commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2581001217)
> I think there will always be a possibility to argue one way or the other,

I agree, so even though I have a different view, it's not that practically important so this shouldn't be a blocker. This PR is an improvement with either approach.

> it seems like it should propagate the error up as a fatal one.

I don't think it needs to be propagated as fatal, the failing subsystem / callsite can just log its issue as a warning (when it has no concept or knowledge on fatality), and then higher
...

💬 stickies-v commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#issuecomment-3601835371)
ACK fa45a1503eee603059166071857215ea9bd7242a

👍 rkrux approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3530005825)
lgtm re-ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b