✅ maflcko closed a pull request: "docs: remove passing CI section in guidelines for PR merging as it's common sense"
(https://github.com/bitcoin/bitcoin/pull/32006)

💬 maflcko commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2822101827)
Closing for now, due to controversy.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2053974854)
In "txgraph: Generalize GetClusterRefs to support subsections (preparation)" 542df1db60af9c2bb27f2b357a8b00817a4bfea0

Maybe check to ensure that `start_pos` is not more than `m_linearization.size()` to prevent out of range access attempt?

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2053878045)
In "txgraph: Add GetMainStagingDiagrams function (feature)" 3fffb59368b3a84973c6d775c349008273d72b91

I've read this comment several times, but I still don't fully understand it.

"or been in a cluster together with modified transaction",

```c++
/** Which transactions have been modified in the graph since creation, either directly or by
* being in a cluster which includes modifications. Only relevant for the staging graph. */
SetType modified;
```
Doesn't this imply that the trans
...

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054497741)
In "txgraph: Introduce BlockBuilder interface (feature)" b76a9b719a9b7ee98b3835aab60369f37f1628a2

This is a bit hard to follow.


💬 maflcko commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2822121445)
Passing one: https://github.com/bitcoin/bitcoin/actions/runs/14580801695/job/40919798022#step:6:2537

`System info: Linux 6.8.0-1021-azure`

Failing one: https://github.com/bitcoin/bitcoin/actions/runs/14580801695/job/40896887951#step:6:2538

`System info: Linux 6.11.0-1012-azure`

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2822126808)
Rebased 720f253abbbeb56872b6c16deee26f3fab842254 -> 4a4eeb94339bb9200012df6a57769dc28e35f553 ([kernelApi_36](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_36) -> [kernelApi_37](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_37), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_36..kernelApi_37))

* Fixed conflict with #32308

🤔 w0xlt reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540)
Concept ACK, as this PR increases the decoupling of components.

nit: The name `SpendBlock` may not be clear (unless this concept is already known and I'm missing the point).
Maybe `BIP30Validation`, something like that, would be clearer.

💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054677515)
nit in 7f368ea62cd89e6ff4238ec49efa2eaddd0e5046: Needs rebase, as it is already included in master. I think the name already implies that this is the "highest sequence number with the given property (non-final)", so the comment seems redundant. The comment seems better to mention how it relates to BIPs. For reference, the existing comment is: `Sequence number that is csv-opt-out (BIP 68)`. See: https://github.com/bitcoin/bitcoin/commit/06439a14c884d7f81f331646ad361e88b3037a51#diff-1698fabd4ddd29
...

💬 w0xlt commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054679774)
```suggestion
bool CConnman::OutboundConnectedToAddress(const std::string& str_addr)
```


The caller already knows that the parameter is a string type, but clarifying what the string represents (a network address) seems better to me.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2054684233)
> @mzumsande, any thoughts on how to treat a timeout after we sent `INV` (no `GETDATA` request)? Treat as send failure and schedule a new connection or treat as successful send and do not schedule a new connection?

The arguments from @andrewtoth make sense to me. I think all situations where we'd like to treat it as a send failure/schedule another connection involve a peer that is either broken in an unusual way or malicious. Malicious peers can act as a black hole, so they can be ignored her
...

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822219540)
> ACK [dda90a9](https://github.com/bitcoin/bitcoin/commit/dda90a96bd01e34f4a564fbd15f1948f1c22ff58)
>
> Nice find!
>
> > external non-ranged descriptor MUST be able to have label (current impl disallows it)
>
> Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?

the test in question:
```
self.log.info("Should import a p2pkh descriptor")
import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),

...

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822221513)
re-based on current master

💬 l0rinc commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822227361)
ACK faca46b0421b568e7e5fefe593420e773d0ec9af

🤔 rkrux reviewed a pull request: "test: fix pushdata scripts"
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2785087703)
Concept ACK

My bad I didn't look at the scripts thoroughly in the previous PR and focussed more on the design of the examples.

I debugged the scripts in the test and can indeed see the unintended script as described in this comment: https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063

```
('encodeable_pushdata1', CScript([OP_DROP, x('96616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616
...

💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054706420)
Shouldn't this one be called `nonstd_2byte_pushdata1` because it uses the `PUSHDATA1` opcode?
That's what is mentioned in the comments of the previous PR and in the spender comment down below.

💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054707416)
I'm guessing it's intentional to have two different ways of creating `CScript` here?
For better illustration.

💬 maflcko commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822268104)
For reference, in the two CI `DEBUG=1` configurations, the bench part seems to take 3x the time. Though, this should be fine, given the motivation in the pull description, and also the fact that there are similarly slow other tests.

💬 TheCharlatan commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054728640)
The comment may be confusing in the context of just using nlocktime, so maybe it should be amended to describe its non-BIP68 meaning too?

💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054735178)
e78e92f6363c4ea1cbedd6631fe56c0cd8e164d5: I don't think this is right. Why the `-1`?

For reference, I debugged this via:

```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 13f6088192..927fcb483c 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -92,6 +92,7 @@ void initialize_chain()
Assert(processed);
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockInd
...