💬 sipa commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017513533)
Done.

💬 sipa commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017513667)
Fixed.

💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2017514437)
good point thanks! fixed in [67d5cc2](https://github.com/bitcoin/bitcoin/pull/32110/commits/67d5cc2a06e6c74e82221d35e2fc03ed6580b240)

💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2017514499)
yea fair -- mentioning the data size doesn't make sense. removed in [6afffba](https://github.com/bitcoin/bitcoin/pull/32110/commits/6afffba34e086de4cf0bb86729e12d116c1dcc9b)

👋 instagibbs's pull request is ready for review: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398)

💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2759328271)
Re-opened due to the recent Great Consensus Cleanup work, though I still think there may be minor debate about the utility of the change.

💬 fjahr commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2759339443)
re-ACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b

Just addressed my comments.

💬 instagibbs commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017525319)
99.9% chance I was running the wrong binary, it instantly crashes for me this time, feel free to resolve

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2017521423)
was this added back by accident?

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017527166)
The within-same-feerate ordering is actually irrelevant as far as the diagram is concerned, it's just a sequence of equal-slope sections in the graph; they could arguably be merged into a single one even.

Still, for consistently/repeatability, I've made it use the size-tiebreaking comparison.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017518773)
I think it's more obviously correct. We should only have `ACCEPTABLE` and `OPTIMAL` ones, but going through all of them shouldn't hurt here.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017515844)
I will think about this. I think there may be a better way to track what we expect to be missing in the diagram.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017520489)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017525668)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017525940)
I have reworded this. Please have a look.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017518589)
That's actually implied by `Assume(m_main_clusterset.m_deps_to_add.empty());` below (but added a comment to clarify).

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017524476)
That's exactly what `BlockBuilder` does? This is test code that simulates the behavior of that (and, also works for staging, which has no index).

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017520354)
I considered something like this, but it complicates matters for the fuzz test, which reconstructs diagram directly using `DepGraph` code, which produces `FeeFrac` ones.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017515353)
The implied ordering is the one that was reconstructed above, through `CompareMainOrder`. I've changed the variable names to make this hopefully clearer.

🤔 glozow reviewed a pull request: "Follow-ups for txgraph #31363"
(https://github.com/bitcoin/bitcoin/pull/32151#pullrequestreview-2723274763)
ACK a52b53926b5c6a5b92255435e3c204cdf18665a2

The third comment was from me misremembering what `FindConnectedComponent` does, my bad. But I think it's quite useful to have a `GetConnectedComponent`.