💬 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
💬 polespinasa commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2732868717)
> If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.
I agree, @ismaelsadeeq maybe we could move this issue to a more "general" one where we migrate all units from BTC/kvB to sat/vB?
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2732868717)
> If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.
I agree, @ismaelsadeeq maybe we could move this issue to a more "general" one where we migrate all units from BTC/kvB to sat/vB?
🤔 l0rinc requested changes to a pull request: "Use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2620965099)
After our last discussion (and with the new AssumeUTXO included) I've rebased and re-reviewed and added examples to my suggestions to simplify the review.
My remaining concerns are that AssumeUTXO doesn't warn in the same way as other dumps do, a few leftover refactoring misses, redundant calculation now that we have a dirty count, missing code coverage for modified code, a leftover memory accounting bug in affected area, and a few code nits to make the change less surprising.
```
diff --
...
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2620965099)
After our last discussion (and with the new AssumeUTXO included) I've rebased and re-reviewed and added examples to my suggestions to simplify the review.
My remaining concerns are that AssumeUTXO doesn't warn in the same way as other dumps do, a few leftover refactoring misses, redundant calculation now that we have a dirty count, missing code coverage for modified code, a leftover memory accounting bug in affected area, and a few code nits to make the change less surprising.
```
diff --
...