💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3408681878)
I have opened a PR with my tests and the left-over nits here: #33636

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434242379)
Fixed.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434242823)
Done, thanks.

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2434247283)
Rebased, added explanation to commit message

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2434247408)
Added explanation to commit message

💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3408692812)
Rebased after https://github.com/bitcoin/bitcoin/pull/32313.

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3408731231)
Rebased after https://github.com/bitcoin/bitcoin/pull/32313, the PR is simpler and better tested this way.

👍 andrewtoth approved a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3342719761)
ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

👍 l0rinc approved a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3312759976)
Left a few code comment suggestions, I think the newly added one is a bit imprecise, but I also don't mind merging as is if others don't think they're serious.

ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2434302649)
```suggestion
* Queue the transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.
```

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2434290529)
Nit: now that they're split properly by type
```suggestion
* This function saves the witness transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.
```
or
```suggestion
* This function saves the transaction wtxid to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.
```

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2434295934)
Given that there are at least 3 gates in the code before the wtxid is actually queued (TxRelay must exist, version handshake must be complete and peer doesn't already have it), it may be more precise to state:
```suggestion
* Initiate a transaction broadcast to eligible peers.
```

📝 l0rinc opened a pull request: "refactor: optimize block index comparisons (1.4-7.7x faster)"
(https://github.com/bitcoin/bitcoin/pull/33637)
### Summary
Profiling of the performance regression in #33618 revealed that `CBlockIndexWorkComparator` and its underlying `base_uint<256u>::CompareTo` are hot paths during block validation, consuming ~4% of CPU time.

### Usage
The comparator is often called directly to compare two separate values and also defines the sorting order for `setBlockIndexCandidates`, a sorted tree containing valid block headers where the comparator is invoked extensively.

### Testing
To ensure the optimized
...

💬 kannapoix commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434307116)
Would it make sense to include listlabels and setwalletflag as well? I assume they were omitted because the parameters are strings but only accept predefined values.

Adding them could help produce consistent errors. For example, with listlabels:
```
$ build/bin/bitcoin-cli -regtest -named listlabels invalid=purpose
error code: -8
error message:
Unknown named parameter invalid
```
Without -named:
```
build/bin/bitcoin-cli -regtest listlabels invalid=purpose
error code: -8
error message:
Invalid
...

💬 kannapoix commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434287898)
Should we add other RPCs that also accept the address_type argument to this list?
Passing a parameter containing ‘=’ to getnewaddress currently returns:
```
$ build/bin/bitcoin-cli -regtest -named getnewaddress label_string include=equal
error code: -5
error message:
Unknown address type 'include=equal'
```

While passing the same to getrawchangeaddress shows:
```
$ build/bin/bitcoin-cli -regtest -named getrawchangeaddress include=equal
error code: -8
error message:
Unknown named parameter inclu
...

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2434355869)
I still think the `std::tie` version is better, pushed it as an alternative as part of a bigger related optimization I had. Added you as coauthor there: https://github.com/bitcoin/bitcoin/pull/33637

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2434358606)
> This doesn't combine well with invalidateblock

Being out of consensus with all outbound peers is a situation as catastrophic as it gets, and probably requires intervention by the user - why wouldn't we want to be a bit spammy in this scenario?
Outside of a real catastrophic chain split, one could of course artificially induce this situation with `invalidateblock` (which is debug-only) and then circle through outbound peers - but I don't see the use case, why would anyone do this and why w
...

💬 sipa commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3408890311)
@mzumsande Agree that sounds sufficiently unlikely to optimize for (and sufficiently expensive to intentionally cause).

Code review ACK on the logic change, but I want to have a look at the tests still.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3408901200)
Rebased due to conflicts.
Removed the first commit. We don't need to modify `EmplaceCoinInternalDANGER`, we can just insert the coins as dirty into the cache. They will be set dirty when they are spent anyways. If a block fails validation inside `ConnectBlock`, then the dirty coins will just rewrite the same value to the db on the next flush or sync.

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3409170060)
- Addresses @instagibbs 's [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432484590) on adding a note for future reviewers.
- Addressed @instagibbs 's [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432451468) on removing the redundant assert.