📝 mzumsande opened a pull request: "test: fix intermittent timeout in p2p_getaddr_caching.py"
(https://github.com/bitcoin/bitcoin/pull/28144)
Fixes #28133

In the consistency check, we not only need to check that our address/port is unique, but that the combination of source and target is. Otherwise the OS may reuse ports for connections on different `-addrbind`, which was happening in the failed runs.

While at it, the second commit cleans up duplicate `getaddr` messages in `p2p_getaddr_caching.py` that do nothing but generate `Ignoring repeated "getaddr"` log messages.

💬 mzumsande commented on issue "p2p_getaddr_caching.py failure in TSan CI":
(https://github.com/bitcoin/bitcoin/issues/28133#issuecomment-1648816185)
See #28144 for a fix.

💬 jonatack commented on pull request "test: fix intermittent failure in p2p_getaddr_caching.py":
(https://github.com/bitcoin/bitcoin/pull/28144#issuecomment-1648834265)
Concept ACK, thank you for looking into it.

🤔 jonatack reviewed a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1544505492)
ACK modulo a few comments

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272913872)
Should now be dropped.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272913933)
Should now be dropped.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272918229)
Seems a shame with the last change to now include this all of this only to access `SIGHASH_DEFAULT = 0`. It might be good to extract the signature hash types to their own unit.

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272920359)
I like how this code is now similar to my suggestion in https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271496876 👍

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272919241)
If you retouch, IIUC per our developer notes the `//!` Doxygen format is for describing a class member or a variable.

`/** ... */` would be for describing a function.

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