💬 hebasto commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2732279914)
> > > 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.
Perhaps I should clarify t
...
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2732279914)
> > > 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.
Perhaps I should clarify t
...
💬 laanwj commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732432018)
i'm hesitant about recommending people to use GNU tools on BSD variants, one of the advantages i hoped cmake would have is that we can just use native build tools as-is.
> macOS ships with GNU Make 3.81 from 2006. This has caused difficult to debug issues, e.g. https://github.com/bitcoin/bitcoin/pull/32070 and https://github.com/bitcoin/bitcoin/issues/30978.
That said, i didn't realize that macOS already ships with GNU make, that changes things.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732432018)
i'm hesitant about recommending people to use GNU tools on BSD variants, one of the advantages i hoped cmake would have is that we can just use native build tools as-is.
> macOS ships with GNU Make 3.81 from 2006. This has caused difficult to debug issues, e.g. https://github.com/bitcoin/bitcoin/pull/32070 and https://github.com/bitcoin/bitcoin/issues/30978.
That said, i didn't realize that macOS already ships with GNU make, that changes things.
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732469202)
@laanwj in the longer run I hope we can "just" move depends over to cmake entirely.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732469202)
@laanwj in the longer run I hope we can "just" move depends over to cmake entirely.
💬 hebasto commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732479269)
> i'm hesitant about recommending people to use GNU tools on BSD variants, one of the advantages i hoped cmake would have is that we can just use native build tools as-is.
That's true.
However, our depends build subsystem is not managed by CMake and requires GNU Make, not BSD Make, to run.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732479269)
> i'm hesitant about recommending people to use GNU tools on BSD variants, one of the advantages i hoped cmake would have is that we can just use native build tools as-is.
That's true.
However, our depends build subsystem is not managed by CMake and requires GNU Make, not BSD Make, to run.
💬 laanwj commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732522102)
> @laanwj in the longer run I hope we can "just" move depends over to cmake entirely.
> However, our depends build subsystem is not managed by CMake and requires GNU Make, not BSD Make, to run.
Ah yes *for the depends* it's unavoidable right now, makes sense, sorry.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732522102)
> @laanwj in the longer run I hope we can "just" move depends over to cmake entirely.
> However, our depends build subsystem is not managed by CMake and requires GNU Make, not BSD Make, to run.
Ah yes *for the depends* it's unavoidable right now, makes sense, sorry.
💬 laanwj commented on pull request "http: Make server shutdown more robust":
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-2732578296)
From what i remember this was *extrememly* hard to get right with libevent (or at least our use of it), without leaking anything or running into race conditions. There have been a few tries to fix the behavior over the years, but they all were different compromises, adding ever more compexity.
As it seems to be where things are heading anyway (#32061) maybe it's better to focus on replacing libevent and its webserver framework wholesale.
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-2732578296)
From what i remember this was *extrememly* hard to get right with libevent (or at least our use of it), without leaking anything or running into race conditions. There have been a few tries to fix the behavior over the years, but they all were different compromises, adding ever more compexity.
As it seems to be where things are heading anyway (#32061) maybe it's better to focus on replacing libevent and its webserver framework wholesale.
🤔 musaHaruna reviewed a pull request: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2693889131)
Concept ACK
Creating a seperate function`test_rpcwhitelistdefault_unset` is okay, it avoids mixing functionalities into the same function, thereby making `test_rpcwhitelistdefault_permissions` less complex.
Each function is doing exactly what the function name suggests. IMO
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2693889131)
Concept ACK
Creating a seperate function`test_rpcwhitelistdefault_unset` is okay, it avoids mixing functionalities into the same function, thereby making `test_rpcwhitelistdefault_permissions` less complex.
Each function is doing exactly what the function name suggests. IMO
💬 musaHaruna commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2000729321)
nit: I think the previous implementation is catching all the edge cases including unintended white spaces.
e.g when I add a space between `"getbestblockhash,getblockcount,"` , it causes the test to fail
` self.users = [
["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],`
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2000729321)
nit: I think the previous implementation is catching all the edge cases including unintended white spaces.
e.g when I add a space between `"getbestblockhash,getblockcount,"` , it causes the test to fail
` self.users = [
["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],`
💬 musaHaruna commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2000733677)
nit: I think the previous implementation is catching all the edge cases including unintended white spaces.
e.g when I add a space between `"getbestblockhash,getblockcount,"` , it causes the test to fail
` self.users = [
["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],`
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2000733677)
nit: I think the previous implementation is catching all the edge cases including unintended white spaces.
e.g when I add a space between `"getbestblockhash,getblockcount,"` , it causes the test to fail
` self.users = [
["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],`
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2000738410)
Thanks!
I've added a commit, which sets Bash as the default shell globally.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2000738410)
Thanks!
I've added a commit, which sets Bash as the default shell globally.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732656120)
The recent feedback on using Bash by default has been addressed.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732656120)
The recent feedback on using Bash by default has been addressed.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2000762116)
Fixed, thanks
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2000762116)
Fixed, thanks
💬 marcofleon commented on pull request "fuzz: Use immediate task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2732695727)
> Happy to include that here
No need, just confirming for my own understanding.
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2732695727)
> Happy to include that here
No need, just confirming for my own understanding.
🤔 musaHaruna reviewed a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2694039878)
Post ACK [52482cb](https://github.com/bitcoin/bitcoin/pull/32033/commits/52482cb24400f8c44ba9628aaaecb7c04b11beb2)
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2694039878)
Post ACK [52482cb](https://github.com/bitcoin/bitcoin/pull/32033/commits/52482cb24400f8c44ba9628aaaecb7c04b11beb2)
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2000800309)
fixed
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2000800309)
fixed
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732777108)
@laanwj I clarified the PR description by adding "This PR does not change the non-depends build."
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732777108)
@laanwj I clarified the PR description by adding "This PR does not change the non-depends build."
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732820174)
> > Both src/minisketch and src/secp256k1 are subtrees. Therefore, they are out of this PR scope.
>
> I don't think they are entirely out of scope? Currently those tests (as well as univalue) are wrapped for (previously in the CI, and still possible using `RUN_UNIT_TESTS`) running under Wine, but you're removing all of that, and the rest of the Wine infra entirely here (no-longer running those tests), or providing a way for anyone to run them locally?
Wine is not a supported platform for B
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732820174)
> > Both src/minisketch and src/secp256k1 are subtrees. Therefore, they are out of this PR scope.
>
> I don't think they are entirely out of scope? Currently those tests (as well as univalue) are wrapped for (previously in the CI, and still possible using `RUN_UNIT_TESTS`) running under Wine, but you're removing all of that, and the rest of the Wine infra entirely here (no-longer running those tests), or providing a way for anyone to run them locally?
Wine is not a supported platform for B
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732825021)
Please let me know if I have left anyone's comment unaddressed.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732825021)
Please let me know if I have left anyone's comment unaddressed.
💬 vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2732857665)
A few notes:
* Bitcoin Core would try to maintain up to 12 outbound connections to peers from all networks that are reachable (IPv4, IPv6, Tor, I2P, CJDNS). From these usually just a few are I2P. So, it is reasonable to assume that from 2,3,4 to 12 I2P connections will be needed. But the case with 12 is when I2P is the only reachable network which is not advisable because of the increased Sybil likelihood.
* This PR is not going to create a storm of I2P sessions creation at startup because
...
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2732857665)
A few notes:
* Bitcoin Core would try to maintain up to 12 outbound connections to peers from all networks that are reachable (IPv4, IPv6, Tor, I2P, CJDNS). From these usually just a few are I2P. So, it is reasonable to assume that from 2,3,4 to 12 I2P connections will be needed. But the case with 12 is when I2P is the only reachable network which is not advisable because of the increased Sybil likelihood.
* This PR is not going to create a storm of I2P sessions creation at startup because
...
💬 fanquake commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732864237)
> Wine is not a supported platform for Bitcoin Core.
If that is the case, then we should stop maintaining workarounds for Win in this codebase. i.e: https://github.com/bitcoin/bitcoin/blob/d61a847af0b4363c228c432556eeadefa5616b3a/test/util/test_runner.py#L161-L163
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732864237)
> Wine is not a supported platform for Bitcoin Core.
If that is the case, then we should stop maintaining workarounds for Win in this codebase. i.e: https://github.com/bitcoin/bitcoin/blob/d61a847af0b4363c228c432556eeadefa5616b3a/test/util/test_runner.py#L161-L163