💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069385247)
Backported to 28.x in #32969.

📝 maflcko opened a pull request: "ci: Enable more shellcheck"
(https://github.com/bitcoin/bitcoin/pull/32970)
shellcheck is often the main "reviewer" of CI code written in Bash, so it seems odd to disable it by putting commands into `bash -c "cmd..."`.

Fix that by removing `bash -c`, where it isn't intended.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069459525)
> `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms.

I wouldn't recommend to do this, because this can lead to missing includes, as also mentioned earlier in https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574. The correct fix is to just add the includes iwyu is asking for.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3069532178)
@User087 There are a few differences from the feature you linked. From the same guide, [the very last section](https://github.com/monero-project/monero/blob/master/docs/ANONYMITY_NETWORKS.md#i2ptor-stream-used-twice):
> I2P/Tor Stream Used Twice
`monerod` currently selects two outgoing connections every 5 minutes for transmitting transactions over I2P/Tor.

The proposed feature here does not use any existing connections for transmitting transactions. It creates a new connection to a random p
...

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069550613)
> The correct fix is to just add the includes iwyu is asking for.

I think this is about cases where IWYU on one platform asks you to add (or remove) an include, and IWYU on other platform asks you to remove (or add) the same include. A reasonable way to resolve these is to decide which IWYU is correct and add pragmas to silence the other IWYU. Other ways to resolve it could be to define mappings, or submit fixes upstream, or choose a single platform to be your canonical IWYU platform and igno
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069564494)
Yes, the keep pragma seems useful, but I personally wouldn't recommend using the no_include pragma in this context

💬 Sjors commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069566904)
Better to point to #6722 as it has the discussion from back then.

💬 stratospher commented on pull request "test: fix intermittent failure in rpc_invalidateblock.py":
(https://github.com/bitcoin/bitcoin/pull/32968#discussion_r2204928129)
yeah makes sense, I guess this was the reason some of the test was moved up in https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2892651335

🤔 instagibbs reviewed a pull request: "cluster mempool: add TxGraph work controls"
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3016273688)
reviewed via `git range-diff master 18e40b6cd5a11e23b7a0654bdd12f08e610ce980 4788c87f7c119f35c551da05e7c43bd5be51420c`

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204933624)
It will not "expend" iterations on oversized levels, so it can return "true" when doing no work.

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204876202)
s/quality/QualityLevel/

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204911282)
The new logic means that you may not have enough to make it considered `ACCEPTABLE` BUT you may end up with `OPTIMAL` anyways and are able to continue until you hit a difficult cluster. In practice this may end up with a lot more clusters known as optimal when their topology is simple.

Might be worth a comment on the "if (!improved) return false;" line

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3069728608)
Maybe an easy fix could be to not pass any env vars, except those with a key specified in `ci/test/00_setup_env.sh`?

🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3016494838)
reviewed through 790f6e7a72d31988c5be9f2d520a04d0c0edfa09

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205033216)
6ce95bffee0f809d9bf4d8fed6f8a7df077fa83f

I think txorphanage_sim subsumes the older `txorphan` harness now unless I'm missing something

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205019742)
do we want to call this after every `AddTx` above instead of after adding all additional announcers?

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205060872)
```Suggestion
assert(m_orphans.size() >= m_unique_orphans);
assert(m_orphans.size() <= m_peer_orphanage_info.size() * m_unique_orphans);
```

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205053496)
d5097d6eef85eab971352cf78fb94a30c6e6f127

looks like you forgot to assert the calculated set matches anything ala

assert(m_peer_orphanage_info == reconstructed_peer_info);

💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3069805059)
[1154618](https://github.com/bitcoin/bitcoin/commit/11546183c70def6c0aa539642fd1c9ada3d46840) to [61e800e](https://github.com/bitcoin/bitcoin/commit/61e800e75cffa256ccdbc2ffc7a1739c00880ce0) - Applied suggestion by @stratospher to move helper methods before their respective subtest. Thanks!

💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2205078542)
While I personally prefer a top-down approach where the main test methods are visible at the top without much scrolling, I am seeing the pattern you described is mostly respected across the tests, so I applied it.