💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772401383)
ms

💬 ariard commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2381410581)
I agree with other reviewers so far that we’re better to analyse a set of replacement rules in a logical whole. Be it a holistic upgrade as what could be achieved in the future e.g cluster mempool or replace-by-feerate with partial overlap in the effects on guaranteeing high-fee block templates lingering in the network mempools. On that regard, see the “[On mempool policy consistency](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021116.html)” email thread from few years a
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1780048541)
I'm open to adjusting it. Are you thinking something like the style of the following?
https://github.com/bitcoin/bitcoin/blob/f83a4f6929c2287bfd26756a03b096da9c974dfd/src/rpc/mempool.cpp#L971-L973

The updates for other comments are queued up locally. Will push after with changes for this comment

🤔 tdb3 reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2335859171)
Thanks for the review @hodlinator! Left some updates.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1780054054)
Thanks! Will update

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1780066327)
Yes, will update the comment to be more consistent

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1780076915)
The original thought/rationale was that the function pertains to the orphanage, which is part of the node (although a `node` argument to the function could enable obtaining the same information).
No file stands out as a perfect fit. Do you feel `test_framework/mempool_util.py` would be reasonable? It seems appropriate since the orphanage and mempool both deal with unconfirmed transactions and are somewhat related.

Let me know and I can update either way.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1780068948)
Agreed, that's more thorough. Will update

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2381411724)
> Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/a481f4ced64b7975. Produces something like this based on the size and from-peers (color) of the orphans.

This looks great!

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780081933)
Exactly, that's what we'd be signalling by reducing the scope

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780081814)
In that case please consider using `std::chrono::milliseconds` instead of multiplying micros by 1000 -making the comment redundant.

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780082276)
There are many such cases, are none of them related to this change?

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780082684)
Yeah, it was messy before, but now that https://github.com/bitcoin/bitcoin/pull/30618/files#diff-d4a2fb26adedc27f16bd3778424fa94c473342a695b228220a1810119028be5bR257-R261 is merged, it might be simpler

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780083894)
So when would this be in scope?

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780084321)
Isn't that what we'd be calculating via `std::ranges::count_if(vInv, &CInv::IsGenTxMsg)`?

🤔 l0rinc reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2335892884)
> It's less productive to request minor changes to something if you haven't decided whether it looks good on a macro level.

I had to reduce the scope of the reviews since I'm not yet familiar with this part of the code.
But I understand it's not what's needed here, so I will let others (having deeper, more holistic experience) do concept reviews.

💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2381424630)
@Zk2u Thanks for the patch! I applied it.

💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2381424649)
> > what are the reasons to be concerned about somebody inspecting the traffic to my node?
>
> * You don't want others (e.g. network admins, internet providers, etc.) to know that you are running a bitcoin node at all. Maybe it's not allowed in your network / country etc., maybe for other reasons such as personal security.

This is what is worrying me - V2-only will not hide the fact that I am running a bitcoin node, but may give the false impression that it does. This is dangerous. My node
...

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780087050)
I was referring to getting rid of the cast, but not important, please resolve these.

✅ russeree closed an issue: "CI : Permission denied error - Multiple failing PRs"
(https://github.com/bitcoin/bitcoin/issues/30998)