📝 Raimo33 opened a pull request: "node: optimize CBlockIndexWorkComparator"
(https://github.com/bitcoin/bitcoin/pull/33334)
## Summary

This PR optimizes `CBlockIndexWorkComparator::operator()` by removing 4 redundant branches.

The previous implementation used multiple separate comparisons with explicit branches for greater-than and less-than cases, resulting in unnecessary code paths.

The new implementation consolidates comparisons into single inequality checks and reduces complexity while preserving its original behavior. This change is particularly beneficial for loading blocks from files and reindexing.

...

💬 Raimo33 commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#issuecomment-3264113178)
Code review ACK 61cde55c56952327d06297a133c30f6877eb62e6

last commit conflicts with https://github.com/bitcoin/bitcoin/pull/33184/ but I'd argue this PR could be merged first.
My current knowledge prevents me from understanding the changes in `test/functional/data/rpc_getblockstats.json` and `test/functional/rpc_getblockstats.py`. Also it seems 433942e840e0b70d5872e5a5f8d84a42fa3ff05a could be split up further

💬 Raimo33 commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#issuecomment-3264116563)
The reason `BlockToJsonVerboseWrite` and `WalletIsMineDescriptors` are slower is due to the fact that they benefit from not computing a comparison twice.

For them specifically, this would be better:
```diff
// First sort by most total work, ...
- if (pa->nChainWork != pb->nChainWork) {
- return pa->nChainWork < pb->nChainWork;
- }
+ if (pa->nChainWork > pb->nChainWork) return false;
+ if (pa->nChainWork < pb->nChainWork) return true;
```

as in the best case scenario it only com
...

📝 sipa opened a pull request: "txgraph: randomize order of same-feerate distinct-cluster transactions"
(https://github.com/bitcoin/bitcoin/pull/33335)
Before, the implicit ordering of transactions in a TxGraph's main graph is sorted by:
- feerate, high to low
- `Cluster`, low to high `m_sequence`
- linearization order within clusters

However, since `m_sequence` values are assigned deterministically and predictably, the ordering of same-feerate distinct-Cluster transactions can reveal something about the order they were inserted into `TxGraph`.

In #28676, this ordering (through `TxGraph::CompareMainOrder`) is used to decide the order o
...

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2328896505)
See #33335.

👍 andrewtoth approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3194671681)
re-ACK 4327327dd07457621fe1a9c88dfbf6fccb0e8854

Changes from my last ACK dbd76e68c3fd86814e96be85a2d23eb542df6e55 (`git range-diff 5d17e64a02...dbd76e68c3 7cc9a08706...4327327dd0`):

- ee75225369: grammar
- ead32f37c8: remove txid conversions to resolve conflict
- 9ec10ceb59: convert `Txid` to `uint256`, update import from `util/transaction_identifier.h` to `primitives/transaction_identifier.h` to resolve conflict
- dbd76e68c3: add functional test for rejecting scheduling private broadca
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2328909174)
grammar nit:
```suggestion
self.log.info("Sending a pair of transactions with the same txid but different valid wtxids via RPC")
```

💬 ajtowns commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2329076580)
```c++
base_uint(const base_uint& b) = default;
base_uint& operator=(const base_uint& b) = default;
```

?

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2329085861)
better imo. more explicit.

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3264520866)
Concept ACK:
the compiler can now generate constexpr constructors and better optimize.

Approach NACK
I think it's more explicit to use the `= default` syntax.

💬 Raimo33 commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-3264550939)
Concept ACK.

will review soon. quick notes in the meantime: I'm a fan of the simpler std::vector approach, I'm not a fan of having a special case for coinbase.

💬 Sjors commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2329241651)
nit: you could use `./build/bin/bitcoin rpc` instead. This removes the need for `-named` elsewhere in the tutorial.

💬 Sjors commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2329248026)
You could also drop `/0/*` because it's not part of the origin. Then later on where you use the xpub, add `/<0;1>/*`.

👍 Sjors approved a pull request: "doc: update multisig tutorial to use multipath descriptors"
(https://github.com/bitcoin/bitcoin/pull/33286#pullrequestreview-3195114640)
utACK 5424b4f1ce74c82b7ae01034bbc1592088048128

I didn't test the tutorial.

💬 Sjors commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3264796807)
Looks like Windows CI is still unhappy:

```
File "D:\a\bitcoin\bitcoin/test/functional/tool_bitcoin.py", line 62, in run_test
self.test_args([], [], expect_exe="bitcoind")
```

Maybe @hebasto has an idea?

💬 l0rinc commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037)
After some usage I agree that we should always just log it, it's not intuitive to see:
```
2025-09-08T05:58:02Z Assuming ancestors of block 00000000000000000000611fd22f2df7c8fbd0688745c3a6c3bb5109cc2a12cb have valid signatures.
...
2025-09-08T05:58:39Z Enabling signature validations at block #738580 (00000000000000000003f5264e65db4f2db7ab624c19e8a27403df3ca069e8ab).
```

where 00000000000000000000611fd22f2df7c8fbd0688745c3a6c3bb5109cc2a12cb is 912683 and yet sigature verification was enab
...

💬 Sjors commented on pull request "RFC: coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3264818576)
Concept ACK

Tested on my M4 macOS machine that is shows the warning. It gets buried in the log, but I think it's a good start, and at least it will help people figure out why their node crashed after the fact.

📝 l0rinc opened a pull request: "validation: log initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336)
### Summary

I noticed this when one of my benchmarks suddenly took several times the original execution and the log lines didn't indicate that somehow script verification got turned on from the beginning (might be related to https://github.com/bitcoin/bitcoin/issues/31494).

### Fix

Always emit the first signature verification state line; other messages like "Assuming ancestors of block ..." do not accurately indicate whether checks are enabled, so users cannot infer the script validatio
...

💬 Raimo33 commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-3264846277)
> The bar should be pretty high for touching such critical code.

I agree but I argue the changes proposed are small enough that this should be seriously considered for merge. the diff makes it hard to grasp, but few actually changed, and for the best.
I'm debated on cae4bb877f9ef1350825ed42a9b97090d5920d39, but for the rest, Concept ACK, Approach ACK, ACK d28302fa2c09c2eac3daad7116dc9812fe01928b.

I've ran the tests and double checked the logic.

```shell
taskset -c 1 ./bench_bitcoin --
...

💬 Sjors commented on pull request "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`":
(https://github.com/bitcoin/bitcoin/pull/33312#issuecomment-3264846807)
ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920

Is it possible to move this into the capnp subdirectory to further narrow the scope of this exception?

💬 Sjors commented on pull request "[DRAFT] Expose AssumeUTXO Load Snapshot Functionality To The GUI":
(https://github.com/bitcoin-core/gui/pull/870#issuecomment-3264907575)
I'm a bit confused about the relation between this and https://github.com/bitcoin/bitcoin/pull/33117. I would suggest rebasing 60f676abf54a454361ae9f5dd17509ec929acbac on that branch for clarity.