💬 janb84 commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2674982705)
tACK,

- Ran unit test
- Ran all the tests
- Validated the code change.

To concider; change title to "must find a fitting solution" in stead of "must find a good solution", but it's a very small nit.

I also wrote a [review note](https://github.com/janb84/Bitcoin-Core_review-notes/blob/f405429fd555efc9dc7194448a08e5f4a75cf63d/reviews/PR-%2331656.md) on the PR to document my learning journey

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965808053)
> Like the check for GetSettingPath seems unnecessar

`WriteSettingsFile` will raise an exception if that check fails, it seems to make a big point of "dynamic settings being enabled".

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965817803)
Done.

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2675005276)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993

💬 ryanofsky commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965819109)
> `WriteSettingsFile` will raise an exception if that check fails, it seems to make a big point of "dynamic settings being enabled".

I think that is ok at least with code above because settings_changed will be false in that case.

💬 fanquake commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2675030838)
> Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.

Thoughts @glozow ?

👍 pablomartin4btc approved a pull request: "ci: Fix filtering out Qt-generated files from `compile_commands.json`"
(https://github.com/bitcoin/bitcoin/pull/31928#pullrequestreview-2633664897)
post-merge ACK d82dc1041524e8402d201d32c9143233b5ec1baf

Verified that the regex adjustment successfully excludes CMake-generated files (`bitcoinqt_autogen/`), as evidenced by the absence of `clang-tidy` runs on these files in the updated [logs](https://api.cirrus-ci.com/v1/task/4775165260726272/logs/ci.log).

💬 fanquake commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2675048687)
@dergoegge want to rebase here and make those changes?

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1965867683)
> IIUC this is basically a way of slowing down the parent resolution of a target package by 2 minutes, thereby increasing the window of possible random eviction when under real cpfp traffic.

Not sure if I understood that right, but I think that would be a different kind of attack where the attacker knows the target packet. What I meant was that that the attacker crafts spam transactions (P/C) that the rest of the network relays, but that end up only in the victim's orphanage, resulting in ev
...

💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675061518)
My Guix build:
```
2bc98ea18648c64ddfbc4136e421e561090c49c2f497cec71ae2fcdd91a36269 guix-build-e181bda061ca/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
3bec23389e5343dadc16ab3ed2bff897519a335ed970f8288521aa7ee7bdf4be guix-build-e181bda061ca/output/arm64-apple-darwin-codesigned/bitcoin-e181bda061ca-arm64-apple-darwin.tar.gz
7f0b7259dc451610040e4fb5f1ef8947770a8fed363c64e00b06870bb5c94607 guix-build-e181bda061ca/output/arm64-apple-darwin-codesigned/bitcoin-e181bda061ca-arm64-apple-
...

🤔 instagibbs reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2612580796)
Looking good 11135464c5c3aef1ce8d2823120467a522ce2c87

I just have conceptual issues with what PostLinerize is guaranteeing us beyond what is explicitly stated in the header file.

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953051233)
> sim real

feel like we're missing a punctuation or I'm unclear what it's saying

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953086802)
1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b

> Cleanup

probably an old reference?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953038482)
micro-nit: seems this should have been renamed in a different commit 1f06bc1e4a8108f1430bcd20fc391c9f663a2e4b

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953094059)
to be clear this is to allow the search space to be split in a bimodal way?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953123706)
do we really not want coverage when the graph is empty?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953324216)
Empty clusters can happen anytime `ApplyRemovals()` is invoked, but not `ApplyDependencies` since that invokes `SplitAll()`, correct?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956309054)
nit
```Suggestion
/** Make a transaction not exist. Transaction must currently exist. */
```

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955027230)
693c3df67dc972dd2fcfbeb8179bcd7833c77bd1

Working on convincing myself that PostLinearization is suffcient to result in optimal clusters. I think it's straight forward that the chunk prefixes before any removed tail transactions are untouched, and once we remove tail transactions, the last touched-but-not-fully-removed chunk may be in a substandard ordering.

So PostLinearization guarantees that the final sub-chunk is reordered to being connected, and that means that sub-chunk is now optima
...

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953287150)
what can we assert about m_to_remove?