💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2822019017)
ACK cc8f239663a0874c307efa30815b40d9ef40badb

👋 maflcko's pull request is ready for review: "refactor: Avoid copies by using const references or by move-construction"
(https://github.com/bitcoin/bitcoin/pull/31650)

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2054580426)
Ok, restored initial version of this pull request. Should be easy to re-review.

💬 achow101 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-2822066744)
NACK, agree with @pinheadmz

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2784838614)
Code review ACK ece168ecef6e57e3a994ec5f7bd341f9c7525721. Changes since last review were squashing and rebasing, doing a better job of passing seed, timeouts, and tmpdirs to child processes. Previously dropped workaround for None errno is also back, this time with link to upstream python bug.

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2054592532)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053031058

I think you could drop all unnecessary encoding and decoding by just dropping `encoding="utf-8"` parameter to `subprocess.run` and printing the bytes directly with something like `sys.stderr.buffer.write(b"<CHILD OUTPUT BEGIN>\n%s\n<CHILD OUTPUT END>" % e.output)`

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2822100971)
Rebased e219f15976f50e0a703bc696b6445feb9950e65d -> eec7c49cf777cdcc53aeb20e31fbd5881c2b008b ([chainman_flush_destructor_4](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_4) -> [chainman_flush_destructor_5](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_4..chainman_flush_destructor_5))

* Rebased to get a CI fix for windows cross

✅ 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