💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772396787)
Considering this out of scope as it is being moved, not introduced.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780058788)
We don't know the number of transaction invs ahead of time, so this could reserve a bunch of extra space that is never used. Unsure the best way to handle this - if it's transactions, the size should generally only be a handful anyway.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780059287)
I think you're referring to the extra space? That's done to leave room for other params to be added in the future, it's more readable.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780063182)
(Was my first instinct too). That requires we pass `txdownload_impl` to the function, and the fuzz target above doesn't have access to that.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780060465)
Agree, changed

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772399108)
I prefer not to assert. My philosophy is that the interface should be simple/robust, i.e. not have a ton of assumptions that would cause the class to fall over if broken. And yes, it means we can easily fuzz that this is the case.

💬 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?