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

💬 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-2675133144)
The `x86_64-w64-mingw32-codesigned` component—including the installer and archived binaries—has been tested on Windows 11 Pro 24H2. All signatures appear correct.

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675167050)
> But it seems that this only picks the binaries. But if I understand the Apple priests correctly, you have to send the whole package.

The directory is passed to `signapple` which will proceed to zip it. Binaries by themselves cannot be uploaded (I tried).

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675183164)
> Note that this only affects code signers so I will hold off on updating signapple in guix for now.

Do you want to include these and then have us ACK-away at the PR?

💬 fanquake commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2675183982)
https://github.com/bitcoin/bitcoin/actions/runs/13458258095/job/37607048581?pr=31923:
```bash
65/139 Test #9: bench_sanity_check_high_priority .....***Failed 78.91 sec
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
/home/runner/work/_temp/src/logging.cpp:337:30: runtime error: implicit conversion from type 'char' of value -20 (8-bit, signed) to type 'unsigned char' changed the value to 236 (8-bit, unsigned)
#0 0x5596549396a8 in
...