💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2440231846)
In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

I don't think this comment is necessary because the tests pass even if the failure cases run first.

💬 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-3415896069)
Backported to `30.x` in #33609.

💬 sipa commented on pull request "refactor: optimize block index comparisons (1.4-7.7x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2440314209)
Mind using `bench.batch(n * n).units("cmp")` or so here, to put the output units in perspective?

💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3415995854)
Related upstream issue: https://codeberg.org/guix/guix/issues/3576

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor (if `internal=false`) & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-3416218823)
> lgtm crACK [664657e](https://github.com/bitcoin/bitcoin/commit/664657ed134365588914c2cf6a3975ce368a4f49)
>
> Good find! Though I find the last paragraph in the PR description slightly confusing:
>
> > If you remove label from external descriptor import object - it will import no problem:
> > Even tho it should NOT, as the descriptor is ranged.
>
> If the label is not present at all, the ranged descriptors can/should be imported. Instead, I find the text in this comment to be helpful
...

💬 fanquake commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#issuecomment-3416250460)
Can you please fix the PR description and commit message? `CMAKE_SKIP_INSTALL_RPATH` is removed from the build system, and `CMAKE_SKIP_RPATH` (which does more than `CMAKE_SKIP_INSTALL_RPATH`) is added to the Guix build. Can you also remove the (what looks like) AI generated content from the PR description and commit message. i.e the bullet pointed list of changes, for a 2 line diff.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440527930)
Note that nothing is written to disk here, this is all happening in-memory. It doesn't look like the actual from-disk behaviour here is easy to test, because that's relying on `BlockManager` internals, so this is probably the closest we can test. (`nStatus` being a non-const public member while `m_dirty_blockindex` being private is... not great)

I think it makes sense to add this as a separate test case, it's not really related to `invalidateblock` and I find targeted tests easier to understa
...

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440542622)
> so if it's just readability that is an advantage now, I have a slight preference for the current approach

Okay, no big deal either way. I'm not sure the performance difference is going to show up in any meaningful way so I generally prefer readability until the need for performance improvement is shown (or obvious), but up to you and either is fine.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440539684)
> because we're iterating through block indices in increasing order of heights

Yes you're right, I hadn't factored this in. Thank you for pointing out my mistake, the logic looks correct indeed.

> makes sense to test this migration behaviour. done in https://github.com/bitcoin/bitcoin/commit/cfadc8038c08f9804c81af7950164483761f1db5.

Thanks, great to have more test coverage!

💬 sipa commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2440626085)
I think this is resolved now, with the merge of #32313?

💬 sipa commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2440648496)
I think the comment is a bit misleading. When calling `FindNestBlocks` (= at the end of this function), LastCommonBlock must be an ancestor of BestKnownBlock, but this isn't the case *during* `FindNextBlocksToDownload` (one of its functions is making sure LastCommonBlock is set to something satisfying that property).

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2440729584)
Yes, I think we can now remove the conditions `!stack.back()->map().contains(op) && !coin.IsSpent()`.

💬 snakamoto129-dev commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3416509771)
El vie, 17 oct 2025, 19:20, Pieter Wuille ***@***.***>
escribió:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/net_processing.cpp
> <https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2440648496>:
>
> > + // Determine the forking point between the peer's chain and our chain:
> + // pindexLastCommonBlock is required to be an ancestor of pindexBestKnownBlock, and will be used as a starting point.
> + // It is being set to the
...

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433879534)
nit: extra commented line

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436027616)
My bad on the linter, lgtm!

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2437193014)
I ended up writing some feerate diagram tests to check my understanding of how it works. It might be helpful for reviewers and/or if you're interested in taking them:

```diff
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index ee8005a1cd2..c86e6a1b5c0 100755
--- a/test/functional/mempool_cluster.py
+++ b/test/functional/mempool_cluster.py
@@ -7,6 +7,8 @@
from decimal import Decimal

from test_framework.mempool_util import (
+ assert_equal_
...

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436684815)
This started failing when I added it to other tests, then I realized it's the wrong direction
```suggestion
assert_greater_than_or_equal(last_val[1] * x['vsize'], last_val[0] * x['fee'])
```

🤔 sipa reviewed a pull request: "p2p: Advance pindexLastCommonBlock early after connecting blocks"
(https://github.com/bitcoin/bitcoin/pull/32180#pullrequestreview-3351389434)
utACK 01cc20f3307c532f402cdf5dc17f2f14920b725b

📝 Raimo33 opened a pull request: "refactor: optimize: reuse containers in transaction policy verification"
(https://github.com/bitcoin/bitcoin/pull/33645)
## Summary

Currently, some methods in `policy.cpp` are inefficiently reallocating containers on each loop iteration.

This PR aims at optimizing policy verifications (transactions sanitization) by reducing redundant heap allocations without losing performance even in worst case scenarios.

These improvements are particularly beneficial during validation, mostly in IBD.

## Details

I moved `std::vector` declarations outside the loop to reduce heap allocations.
This change allows the
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2440919133)
Yes. Up to block 381999 on mainnet.

Parallel run, ~20 minutes:

2025-10-16T13:58:05Z initload thread exit
2025-10-16T13:58:35Z Syncing txindex with block chain from height 189999
2025-10-16T13:59:12Z Syncing txindex with block chain from height 211999
2025-10-16T13:59:42Z Syncing txindex with block chain from height 223999
2025-10-16T14:00:13Z Syncing txindex with block chain from height 235999
....
2025-10-16T14:16:01Z Syncing txindex with block chain from height 373999
2025-10-16T1
...