💬 CaseyCarter commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1649153185)
Apologies for forgetting to keep the PR squashed. I've merged the two commits into one.

💬 real-or-random commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1649178174)
> GitHub Actions: [2000](https://github.com/pricing) minutes/month

AFAIU public repos are free (no limit) for public repos: https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1649242481)
Updated 42233bea28f6f7c464f0cd6499d675e81b3e8512 -> c5fac53bca920626843723869a7677cef639df4d ([kernelRmUnivalue_6](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_6) -> [kernelRmUnivalue_7](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_6..kernelRmUnivalue_7))

* Addressed @jonatack's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272913872) and [comment](https://g
...

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273093134)
I'll leave this for a follow-up.

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1649399453)
Looks like calling the armhf (cross) compiler (which happens often) via qemu-aarch64 on amd64 is too much overhead and times out? I'll try to run on aarch64 metal directly, which should avoid the extra hop through qemu in the CI.

👍 vasild approved a pull request: "test: fix intermittent failure in p2p_getaddr_caching.py"
(https://github.com/bitcoin/bitcoin/pull/28144#pullrequestreview-1544952356)
ACK e4d37875e6a440748623250d77ec3be523a1d34d

Today I learned something new: it is possible to have duplicate (srcaddr, srcport) for TCP connections if the destination is different. Thank you!

💬 vasild commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273212480)
nit, missing white space after `=`:
```suggestion
dst_addr_and_port = f"{p2p_conn.dstaddr}:{p2p_conn.dstport}"
```

💬 vasild commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#discussion_r1273209890)
There is a similar pattern below on lines 108,110,112. Should be possible to remove those too? If yes, then also `from test_framework.messages import msg_getaddr`.

To other reviewers: the `GETADDR` message is sent at the end of `P2PInterface::on_version()`.

💬 vasild commented on issue "p2p_getaddr_caching.py failure in TSan CI":
(https://github.com/bitcoin/bitcoin/issues/28133#issuecomment-1649411695)
It is good to put asserts that can never the triggered because sometimes they are.

📝 fanquake opened a pull request: "valgrind: add suppression for bug 472219"
(https://github.com/bitcoin/bitcoin/pull/28145)
Now that https://bugs.kde.org/show_bug.cgi?id=472219 has been fixed upstream in:

https://sourceware.org/git/?p=valgrind.git;a=commit;h=6ce0979884a8f246c80a098333ceef1a7b7f694d

Add a supression to ignore the bug until we are using a fixed version of Valgrind.

Related to #28072.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1649465874)
> Maybe as a followup we could add some pedantic checks in init to make sure they're sane values that fit into 32bits and make max_orphan_txs a uint32_t. Please ignore if those checks exist and I'm not seeing them.

Thanks for pointing this out. I agree that some sane limits/checks would make sense for these settings as users could shoot themselves in the foot otherwise. I will leave this for a follow-up though as this isn't a new problem.

💬 MarcoFalke commented on pull request "valgrind: add suppression for bug 472219":
(https://github.com/bitcoin/bitcoin/pull/28145#issuecomment-1649478876)
lgtm ACK 50f7214e0915a88dd81c1ac1d292e049a398cda2

Didn't test

💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1649479100)
> What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

I agree with this part. I think the solution should be expanded to give a chance in such cases, and it should be merged within the same PR.

💬 naumenkogs commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1649481055)
ACK f5d17e1a3f2856483ad3a6abcf3ea415f2d05fde

🚀 fanquake merged a pull request: "net processing, refactor: Decouple PeerManager from gArgs"
(https://github.com/bitcoin/bitcoin/pull/27499)

👍 fanquake approved a pull request: "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS"
(https://github.com/bitcoin/bitcoin/pull/28124#pullrequestreview-1545063455)
ACK faa8c1be265d2344a3bc0932455b0182ec7d64c7

🚀 fanquake merged a pull request: "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS"
(https://github.com/bitcoin/bitcoin/pull/28124)

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1273298422)
I think this catch now needs to go?

💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1649517478)
Consider when a malicious Internet provider or something can request the `forceinbound` IP address.

Does this PR open any fundamentally new risk in this case? I'm thinking of disrupting the network by causing many disconnections if the `forceinbound` practice is broad.

This probably depends on whether it's possible to take multiple slots from the same `forceinbound` IP (or, rather, initiate multiple evictions), which I'm not 100% sure about.

If my suspicions are correct, this might bet
...

💬 MarcoFalke commented on pull request "ci: Start with clean env":
(https://github.com/bitcoin/bitcoin/pull/27976#issuecomment-1649519608)
Split out an easy-to-test bugfix: https://github.com/bitcoin/bitcoin/pull/28138 . So please review that one first.