💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434220844)
In this commit, `chunk_feerate.fee` is used further down, but this gets cleaned up further in #33591 with the introduction of `CFeeRate::GetFeePerVSize`, at which point this could be inlined. I'll make that change in #33591.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434234855)
Will take this doc change in #33591.

📝 fjahr opened a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636)
This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the first commit.

The following commits address a few left-over nit comments that didn't make it in before merge.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434241333)
Good point! Taking your change.

💬 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