💬 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

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714910)
Directly checked cluster size instead.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714959)
This should only be called when not `test_accept`, right?

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633714993)
removed the static cast for `m_conflicting_fees` since that's the same type as `m_total_modified_fees`, but left the other static casts in place since explicit is better than implicit

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715042)
yep, rebased + removed

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715153)
great, added a little commentary on top and repeated this for 2nd package

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715573)
sure, taken

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633715847)
adapted more of the test to use this value

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

💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1633721111)
> I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.

See https://github.com/mzumsande/bitcoin/commit/db8232d47068bba9ae1bb8135ef456be731e5ca5 - because of the inheritance the bool `interrupted_replay` needed to be added to quite a few spots, which I didn't love.

> I think it would be pretty hard to look at the code after this
...

💬 maflcko commented on pull request "test: add coverage for errors for `combinerawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/30264#issuecomment-2159123696)
lgtm ACK ab98e6fd03970d6b5a593674c84e762a47b90ea6

👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2108566302)
Code review ACK 65abc0b5b54edb06cc3bf584691beddbeb802ddc with caveat that I don't know enough about cirrus and github configuration to be very confident in the changes. But they do make sense and appear to do what's described.