💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633421463)
I think it is fine to have this log, but happy to review a separate pull removing it :)

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633423562)
This reminded me to change the two other log lines before `fatalError` to use the error level. So I pushed that other change in the last push.

💬 mzumsande commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1633424948)
The primary source of that information is `CNode::m_conn_type`. Is it necessary to have a partial copy of that in `Peer`?
If not, an alternative approach might be to use that, as is done for various other spots in net_processing that call `IsInboundConn()`. We might need also need helper functions similar to `GetExtraBlockRelayCount` but for inbounds. I haven't tried it or looked into it in depth, but just wanted to bring up where you've considered that option.

⚠️ Sjors opened an issue: "Add bitcoind and bitcoin-cli to macOS release"
(https://github.com/bitcoin/bitcoin/issues/30262)
### Please describe the feature you'd like to see added.

Currently the macOS downloads only contain bitcoin-qt.

It would be nice to have the other binaries as well, especially `bitcoin-cli`. Currently the user has to compile these themselves.

### Is your feature related to a problem, if so please describe it.

E.g. for a Stratum v2 workshop the instructions for macOS have to completely different than for Linux, because the latter can use the command-line. https://github.com/plebhash/sv2-wor
...

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1633492530)
I split f375fb9a0dff7ccead80626f4aee853db7799e6f in two parts:

1. 6d6ddcb759465c7375ff772faa960cf92805468a simply adds and uses the interface
2. 6b7c8c3e27ae529306d18019e7bd94b4ae5b9a21 drops the `BlockAssembler` constructor without `options` argument

I changed this second commit to keep `ApplyArgsManOptions`.

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1633492626)
Done

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1633492703)
I think that was by accident. Moved it back.

👍 tdb3 approved a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2108210705)
ACK 0000276b31cea5e443a59d94a98c569293ada951

👍 theStack approved a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2108226640)
Code-review ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883

💬 paplorinc commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#issuecomment-2158816952)
Thanks for the review @josibake, I've added the conclusions of the benchmarks to the commit message (but copied it to the PR description as well now), can you please check if https://github.com/bitcoin/bitcoin/pull/30093/commits/ebf8667f634653a2eebe404469bfb62c5bb3add2 answers your questions?

@theuni, appreciate your previous reviews and insights, please take a look at the changes I did since.

📝 maflcko opened a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263)
Most supported operating systems ship with clang-16 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.

For reference:
* https://packages.debian.org/bookworm/clang-16
* https://packages.ubuntu.com/noble/clang (clang-18)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang18`); No idea about OpenSus
...

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1633575260)
We do, at least to my understanding, given some `PeerMan` methods that are not called with any `CNode` reference need to check whether the peer that sent us the data they are processing is inbound or outbound.

A good example is `PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs`, which is called from `PeerManagerImpl::BlockChecked` and is passed a `NodeId` grabbed from `mapBlockSource`

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2158906175)
This is needed for stuff:

* `std::ranges::equal`, see https://github.com/bitcoin/bitcoin/pull/29071
* `std::source_location`, see https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1851949647
* ...

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2158932014)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2129147575

> I wonder whether a malformed call to `MakeWalletLoader` should shut down the node at all,

Thanks for reporting this. I'm confused how it happens because `MakeWalletLoader` should be called from the node process (via `Init::makeWalletLoader`method) and it should run in the wallet process. So if it crashes, it should kill the wallet process, not the node process. If you can post more details about what you found at h
...

💬 mzumsande commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1633625118)
Yes, but we could attempt to retrieve the `CNode` by `NodeId` from `m_nodes`, as the current code does it for `Peer` from `m_peer_map`.
I think the lookup would be `O(N)` for iterating through the vector `m_nodes` instead of `O(log N)` for the `std::map`, so maybe that is the reason for the redundancy?

💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1633646409)
Umm, I really cannot see why I wanted to do this in two steps. It really doesn't seem necessary.

I've also re-arranged the code a bit to minimize the diff

💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1633656796)
Updated to:

(should only happen with blocks loaded from disk, as those share the same nSequenceId: 0 for blocks on the best chain, 1 for all others)

📝 brunoerg opened a pull request: "test: add coverage for errors for `combinerawtransaction`"
(https://github.com/bitcoin/bitcoin/pull/30264)
This PR adds test coverage for the following errors for the `combinerawtransaction` RPC:

* Tx decode failed
* Missing transactions
* Input not found or already spent

For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1633665883)
Yeah, that'd make sense. I don't think increasing the lookup complexity for this, even if we are talking about a vector of hundreds of nodes at most, is worth not keeping a boolean in `Peer`

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714824)
taken with only light modifications