💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2216025047)
I have opted not to do this in the follow-up PR.

🚀 fanquake merged a pull request: "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects"
(https://github.com/bitcoin/bitcoin/pull/32868)

💬 glozow commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2216047704)
67837bb426049918004fe48525db71b515940e6f seems like an unnecessarily complex approach? We can just keep a set of `Wtxid`s and, if the peer does not support wtxidrelay, grab the txid from `info.tx` (which we have to look up anyway) to construct the inv message. It's also going to be Wtxids pretty much 100% of the time.

💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3089499548)
(rebased)

📝 ismaelsadeeq opened a pull request: "test: fix `ReadTopologicalSet` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33007)
This PR is a simple fix for a potential unsigned integer overflow in ReadTopologicalSet.
We obtain the value of `mask` from fuzz input, which can be the maximum representable value.
Adding 1 to it would then cause an overflow.

The fix skips the addition when the read value is already the maximum.

See https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569 for more context

💬 ismaelsadeeq commented on pull request "test: fix `ReadTopologicalSet` unsigned integer overflow":
(https://github.com/bitcoin/bitcoin/pull/33007#issuecomment-3089561933)
cc @maflcko @sipa

💬 sipa commented on pull request "test: fix `ReadTopologicalSet` unsigned integer overflow":
(https://github.com/bitcoin/bitcoin/pull/33007#issuecomment-3089571341)
utACK

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3089571542)
> we wouldn't propagate invalid blocks (this does not really matter that much but why do it if we can avoid it)

💬 sipa commented on pull request "test: fix `ReadTopologicalSet` unsigned integer overflow":
(https://github.com/bitcoin/bitcoin/pull/33007#issuecomment-3089573020)
utACK

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3089593571)
> we wouldn't propagate invalid blocks (this does not really matter that much but why do it if we can avoid it)

Thinking about this some more, I don't think this particular point is persuasive. Invalid blocks are propagated quite easily through compact blocks' high bandwidth connections already.

1. If all txns are known to the receiving peer, the block is forwarded
2. If txns are missing, they are requested, the sending peer unconditionally responds to these messages, and the block is thu
...

💬 ArmchairCryptologist commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3089611738)
In related news, mempool.space seems to have recently reduced their minimum feerate to 100 sats/vB which allows for increased visibility in how these behave. Broadcasted transactions below 1000 sats/vB do in fact seem to actually get mined by both Antpool and F2pool (and possibly others). See for example blocks [906076](https://mempool.space/block/00000000000000000001fabd9e42c57352f2883c0f00c953f6bb4aaa9465587e) and [906079](https://mempool.space/block/0000000000000000000224057fe587e2b3fbcc6387c
...

💬 danielabrozzoni commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3089611995)
@stickies-v @brunoerg thank you for the suggestion!

I agree, I think renaming makes the code clearer and reduces the chances of accidentally using the uncached version and leaking addrman addresses :)

I updated the PR title/description and pushed a commit to rename GetAddresses to GetAddressesUncached!

📝 instagibbs converted_to_draft a pull request: "p2p: Relax BlockRequestAllowed to respond to advertised blocks"
(https://github.com/bitcoin/bitcoin/pull/32869)
First commit adds coverage to a similar case to #13370 . h/t dergoegge for detecting this issue independently. Documentation of this behavior with a test is then followed by a proposed change where anytime we considered something `VALID_TRANSACTIONS` and potentially advertised via compact blocks, we should respond to requests for the `getdata` if the peer fell back for whatever reason.

If the second commit is deemed controversial it can be shelved and current behavior documented alone.

✅ instagibbs closed a pull request: "p2p: Relax BlockRequestAllowed to respond to advertised blocks"
(https://github.com/bitcoin/bitcoin/pull/32869)

🤔 maflcko reviewed a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3033620723)
looked at c126475ed7a17ec9030066056e31846c7124dcf~20 💇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: looked at c126475ed7a17ec903006
...

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216100851)
nit in e5295f7ad64be1d834d3e0c2726edfcc67debfff: What about DANGER_DOCKER_BUILD_CACHE_HOST_DIR below?

Could link to the reverted commit in the commit description instead: `e87429a2d0f23eb59526d335844fa5ff5b50b21f`?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216093011)
nit in 20a258310ea4fe1c4cd280d26047aee063215454: Buildx always uses BuildKit. Can remove buildkit stuff.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216152725)
nit in 868ddf557180ffbc7a60ed539a5df1e1cd3e757c: this seems a bit fragile and won't work for monotree repos ( bitcoin-core/gui), and won't work for forks that accept pull requests.

Seems better to just hard-code this to cirrus, possibly add a comment: `# Run on cirrus by default, but in forks this can be replaced by 'ubuntu-latest' to run on GHA`

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216163712)
nit in 868ddf5: Yeah, seems required. Same for the other caches?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2216107839)
nit in e5295f7ad64be1d834d3e0c2726edfcc67debfff: The script has tracing enabled, so i don't think this is needed?