💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706946286)
Yeah I like your approach better, taken, thanks.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707005179)
Yeah that'll work, ~taken, thanks.

👍 paplorinc approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2225466836)
ACK d045fc7ac1729cf29140c43518f20375f2aaa1cc

Note that I don't know the second order effects of constraining previous public input values, somebody else should assess that part

💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2273685985)
Running this exposes that nodes are currently making duplicate transaction requests not too infrequently. ::facepalm:: What happens is that when Peer1 announces {parent,child} and Peer2 announces {parent,child} and we request parent from 1 and child from 2, but 2 responds faster, then orphan handling will request parent by txid from 2. Then it gets the parent by txid from 2. Then when peer 1 replies to the request for parent (sending a duplicate transaction), the current behavior of this PR f
...

👍 tdb3 approved a pull request: "ci: Silent Homebrew's reinstall warnings"
(https://github.com/bitcoin/bitcoin/pull/30591#pullrequestreview-2225476443)
cr ut ACK 032ebe5be4dfc3eb3d3693f1284fac6458bacbf3

💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273721994)
I think there was a slight miscommunication. My reply was to your point of a "large fork", whose check (currently) happens after the chain params are asked for the hash: https://github.com/bitcoin/bitcoin/blob/676abd1af754964858a60fddffb9a12c0a6c2749/src/validation.cpp#L5663 .

You also raised a different point about a user downloading a too recent snapshot. This is a different scenario, however the same check in validation will hit it currently. I agree that the error message can be improved
...

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273742263)
> Thanks for the review everyone. Maintainers: there is still a determinism issue here as per Niklas, which i have yet to re-reproduce.

Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205 ?

Even if not, I wonder if stability is a hard blocker for a fuzz target. Obviously, it should be fixed, but this can be done in a follow-up, if the fuzz target otherwise makes sense to merge.

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2273780176)
reviewed through https://github.com/bitcoin/bitcoin/pull/30286/commits/06ea1016ff40ebcf3e668bdb993a0747904cb40f

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1706029402)
in 29cac3e85b23f1264121cbd50e4a09755b0c8352

I think this commit may benefit from being two smaller commits, one that demonstrates the caching, and a second that demonstrates using it to skip the splitting computation?

I do not understand what it's doing, as-is.

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705789091)
I do realize the argument changes later, so maybe this just on the later commit
```Suggestion
queue.emplace_back(/*inc=*/SetInfo<SetType>{}, /*und=*/std::move(component), /*pot_feerate=*/FeeFrac{});
```

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707280073)
why 64?

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707294690)
why 4?

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705747742)
I think this can be used with DepGraphFormatter during `Unser` with reversed `reordering`

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1704526813)
*non-empty* topological subsets?

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705913641)
I would have appreciated slightly more pedantic comments demonstrating how the computation matches the comments :sweat:

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705666485)
move of `m_todo` instantiation seems like a no-op?

🤔 TheCharlatan reviewed a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2223960433)
lgtm , I hope we can finally get this project under way now.

💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707213519)
Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?

💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706885348)
If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?

💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253)
Nit: Could be declared `static`?