💬 tnndbtc commented on pull request "test: fix intermittent failure in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2731389365)
@mzumsande Regarding to your concerns on `disconnect_p2ps`, I took a look at the implementation of it in `test/functional/test_framework/test_node.py`:
```
def disconnect_p2ps(self):
"""Close all p2p connections to the node.
Use only after each p2p has sent a version message to ensure the wait works."""
for p in self.p2ps:
p.peer_disconnect()
del self.p2ps[:]
self.wait_until(lambda: self.num_test_p2p_connections() == 0)
```
...
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2731389365)
@mzumsande Regarding to your concerns on `disconnect_p2ps`, I took a look at the implementation of it in `test/functional/test_framework/test_node.py`:
```
def disconnect_p2ps(self):
"""Close all p2p connections to the node.
Use only after each p2p has sent a version message to ensure the wait works."""
for p in self.p2ps:
p.peer_disconnect()
del self.p2ps[:]
self.wait_until(lambda: self.num_test_p2p_connections() == 0)
```
...
💬 davidgumberg commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2731477125)
Tested and Reviewed ACK 6869fb417096b43ba7f7
I could be wrong, but I think https://github.com/bitcoin/bitcoin/commit/0f4e20728d231935f0140845370090a760ae93af was more correct (see https://github.com/bitcoin/bitcoin/pull/32073#discussion_r1999868812), but I don't think it's that important.
Anecdotally, more likely to occur on startup, probably because of how many new connections we're making, so I used this script to reproduce:
```bash
bitcoind -debug=net -daemonwait && sleep 20 \
&&
...
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2731477125)
Tested and Reviewed ACK 6869fb417096b43ba7f7
I could be wrong, but I think https://github.com/bitcoin/bitcoin/commit/0f4e20728d231935f0140845370090a760ae93af was more correct (see https://github.com/bitcoin/bitcoin/pull/32073#discussion_r1999868812), but I don't think it's that important.
Anecdotally, more likely to occur on startup, probably because of how many new connections we're making, so I used this script to reproduce:
```bash
bitcoind -debug=net -daemonwait && sleep 20 \
&&
...
💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2000090721)
Good catch, will address in a refactor follow-up, or if I touch here again.
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2000090721)
Good catch, will address in a refactor follow-up, or if I touch here again.
🚀 fanquake merged a pull request: "ci: [lint] Use Cirrus dockerfile cache"
(https://github.com/bitcoin/bitcoin/pull/31948)
(https://github.com/bitcoin/bitcoin/pull/31948)
💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731548516)
> > The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
>
> in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely.
Okay, I think this branch will break building on NetBSD, and shouldn't be merged as-is, I'll follow up.
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731548516)
> > The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
>
> in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely.
Okay, I think this branch will break building on NetBSD, and shouldn't be merged as-is, I'll follow up.
💬 fanquake commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731552311)
> I'll follow up.
Do you want to split out 7d34c19853e7a5528d69c5f30580e7e9712e61f0? That can be merged now.
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731552311)
> I'll follow up.
Do you want to split out 7d34c19853e7a5528d69c5f30580e7e9712e61f0? That can be merged now.
📝 davidgumberg opened a pull request: "ci: Drop ENABLE_HARDENING=OFF from clang-tidy"
(https://github.com/bitcoin/bitcoin/pull/32087)
Split out from #32071
It's not clear why this was added in the first place, but it is not necessary currently.
https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193 https://github.com/bitcoin/bitcoin/pull/24753.
(https://github.com/bitcoin/bitcoin/pull/32087)
Split out from #32071
It's not clear why this was added in the first place, but it is not necessary currently.
https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193 https://github.com/bitcoin/bitcoin/pull/24753.
💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731605465)
> Do you want to split out [7d34c19](https://github.com/bitcoin/bitcoin/commit/7d34c19853e7a5528d69c5f30580e7e9712e61f0)? That can be merged now.
Opened https://github.com/bitcoin/bitcoin/pull/32087
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2731605465)
> Do you want to split out [7d34c19](https://github.com/bitcoin/bitcoin/commit/7d34c19853e7a5528d69c5f30580e7e9712e61f0)? That can be merged now.
Opened https://github.com/bitcoin/bitcoin/pull/32087
👍 fanquake approved a pull request: "ci: Drop ENABLE_HARDENING=OFF from clang-tidy"
(https://github.com/bitcoin/bitcoin/pull/32087#pullrequestreview-2692923853)
ACK 7d34c19853e7a5528d69c5f30580e7e9712e61f0
(https://github.com/bitcoin/bitcoin/pull/32087#pullrequestreview-2692923853)
ACK 7d34c19853e7a5528d69c5f30580e7e9712e61f0
🚀 fanquake merged a pull request: "ci: Drop ENABLE_HARDENING=OFF from clang-tidy"
(https://github.com/bitcoin/bitcoin/pull/32087)
(https://github.com/bitcoin/bitcoin/pull/32087)
💬 fanquake commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000261713)
s/Bitcoin/Bitcoin Core/
> either on your local machine or through cross-compilation
I think this needs to be rephrased, or just dropped.
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000261713)
s/Bitcoin/Bitcoin Core/
> either on your local machine or through cross-compilation
I think this needs to be rephrased, or just dropped.
💬 fanquake commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000262395)
Do we need to copy-paste things here, given you've already linked to somewhere else that already has the same info? In either case, `pkgconf` isn't needed with depends, so should be dropped.
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000262395)
Do we need to copy-paste things here, given you've already linked to somewhere else that already has the same info? In either case, `pkgconf` isn't needed with depends, so should be dropped.
💬 jsarenik commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2000274205)
Yes, it definitely looks like a stray leftover debug statement. No, I did not write a single line of Python code here. Maybe @ajtowns or @kallewoof may comment on that line.
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2000274205)
Yes, it definitely looks like a stray leftover debug statement. No, I did not write a single line of Python code here. Maybe @ajtowns or @kallewoof may comment on that line.
💬 jsarenik commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2000344600)
Apologies for a stray comment. I was out of context "thanks" to GitHub app on Android.
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2000344600)
Apologies for a stray comment. I was out of context "thanks" to GitHub app on Android.
🚀 fanquake merged a pull request: "test: Update coverage.cpp to drop linux restriction"
(https://github.com/bitcoin/bitcoin/pull/32059)
(https://github.com/bitcoin/bitcoin/pull/32059)
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000408118)
The main install instructions pull in libevent, boost (and pkgconf) which are not needed for a depends build. That's not going to be obvious, as demonstrated by the fact that I didn't know `pkgconf` isn't needed :-) (which I'll drop)
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000408118)
The main install instructions pull in libevent, boost (and pkgconf) which are not needed for a depends build. That's not going to be obvious, as demonstrated by the fact that I didn't know `pkgconf` isn't needed :-) (which I'll drop)
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000410100)
Apparently `pkgconf` isn't needed, see https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000262395
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000410100)
Apparently `pkgconf` isn't needed, see https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000262395
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000416335)
I pushed a variation.
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2000416335)
I pushed a variation.
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2732019454)
Will rebase when the base PR does so, or is merged.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2732019454)
Will rebase when the base PR does so, or is merged.
💬 vasild commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2732053677)
The documentation that I linked is explicit:
https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
> _GLIBCXX_ASSERTIONS
Defined by default when compiling with no optimization, undefined by default when compiling with optimization. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.
If you suspect that there is a bug in GCC or the documentation is
...
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2732053677)
The documentation that I linked is explicit:
https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
> _GLIBCXX_ASSERTIONS
Defined by default when compiling with no optimization, undefined by default when compiling with optimization. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.
If you suspect that there is a bug in GCC or the documentation is
...