💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595372132)
nit
```suggestion
const auto block_data{chainman.m_blockman.ReadRawBlock(pos, block_part)};
```

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595372728)
nit
```suggestion
if (const auto data{blockman.ReadRawBlock(pos)}) return *data;
```

✅ b-l-u-e closed a pull request: "[p2p] Fix signed integer overflow in LocalServiceInfo::nScore"
(https://github.com/bitcoin/bitcoin/pull/33072)

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2595408520)
Done.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2595409347)
Nice catch, done both.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3621078014)
I have reorganized this PR, dropping most optimizations in favor of a future follow-up.

Addressed comments, added more explanations, and made a few code simplifications.

A python version of the algorithm, lacking most optimizations and using simpler data structures than the C++ version, can be found in https://gist.github.com/sipa/822f60db6608a26bb4aae75fd31bcb12 (in sfl.py).

📝 sipa opened a pull request: "Optimized SFL cluster linearization"
(https://github.com/bitcoin/bitcoin/pull/34023)
Follow-up to #32545, part of #33629.

This contains many of the optimizations that were originally part of #32545 itself.

👍 sedited approved a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950#pullrequestreview-3548327547)
Nice, ACK 7b90b4f5bb10e2156709b07e3996f867e2421232

Also did a guix build.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3621171277)
Verified via `git range-diff 2d398050ee31db05e9222149b5e60572ac31d803...f391296b17a755153960e0afc736df37d1d5fb1e` since my last ack.
It contains the suggested comment adjustments, const brace inits, include without comment, fake test ADDRMAN_ADDRESSES, missing coverage for `nonexistent_recipient` confirmation.

code review ACK f391296b17a755153960e0afc736df37d1d5fb1e

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595516221)
In this case I think the nesting's fine, it signals clearly that `opt_size` should only be checked if `opt_offset` was already successful - in this version we have to check the return values. Both are fine, I'm just explaining why we may not want to avoid the nesting here.

📝 mossein opened a pull request: "rpc: Reject distinct transactions in combinerawtransaction"
(https://github.com/bitcoin/bitcoin/pull/34024)
Previously, combinerawtransaction silently processed only the first transaction when given unrelated transactions as input, ignoring the rest. This could be confusing and lead to unexpected behavior.

This change adds validation to ensure all transactions passed to combinerawtransaction have the same base structure (same inputs and outputs), throwing RPC_INVALID_PARAMETER if they differ.

Fixes #25980

<!--
*** Please remove the following help text before submitting: ***

Pull requests
...

💬 mossein commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-3621298976)
Closing in favor of #31298 which has been in review since November 2024 and addresses the same issue. I wasn't aware of the existing PRs when I opened this. I'll contribute by reviewing #31298 instead.

Apologies for the duplicate!

✅ mossein closed a pull request: "rpc: Reject distinct transactions in combinerawtransaction"
(https://github.com/bitcoin/bitcoin/pull/34024)

💬 mossein commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3621302427)
Tested ACK a26e9fd5b5908853d37b81915f5e12a5ad822f86

Built on macOS, ran
rpc_createmultisig.py
- passes. The approach of stripping scriptSigs/scriptWitnesses and comparing txids is clean.

Agree with the minor nits from other reviewers (style changes, error message verbosity).

🤔 furszy reviewed a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3548499753)
utACK 5646e6c0d3581f12419913b88745f51c7a3161b9

📝 ajtowns opened a pull request: "net: Waste less time in socket handling"
(https://github.com/bitcoin/bitcoin/pull/34025)
Cuts out some wasted time in net socket handling. First, only calculates the current time once every 50ms, rather than once for each peer, which given we only care about second-level precision seems more than adequate. Second, caches the value of the `-capturemessages` setting in `CConnman` rather than re-evaluating it every time we invoke `PushMessaage`.

💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3621332052)
Not sure if the flame graph is usable, but:


GetBoolArg takes up 0.31% of total time, as part of PushMessage that takes up 1.75% of total time, in b-msghand.

GetTime takes up 0.82% of total time, as part of InactivityCheck that takes up 1.78% of total time, in b-net.

Converting from std::chrono::microseconds to NodeClock::time_point is a lot more intrusive (impacting at least net_processing and no
...

💬 sedited commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3621358666)
Concept ACK

💬 ajtowns commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3621363780)
Seems fine to test it if we have code to export it over RPC.

I don't think I've ever seen it be useful for debugging anything, and just knowing the height without knowing if it's the same header you have for that height or what the chainwork is doesn't seem very useful. So dropping `peer->m_starting_height` entirely (and only reporting it in the `receive version message` debug log entry) might be better?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2595703803)
This is tested now by having zero worker threads.