💬 philmb3487 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2666829924)
@sipa Thanks for bringing this up. Apple moved the exFat filesystem to userspace and it had nasty side effects. What a terrible idea, what is Apple even thinking?
To be sure, have other drivers been moved to user-space?
For all we know they might also be broken. It needs to be checked, it seems doubtful they would only move one driver over.
On Tue, Feb 18, 2025 at 21:00, l0rinc ***@***.***(mailto:On Tue, Feb 18, 2025 at 21:00, l0rinc <<a href=)> wrote:
> @l0rinc commented on this pull request.
...
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2666829924)
@sipa Thanks for bringing this up. Apple moved the exFat filesystem to userspace and it had nasty side effects. What a terrible idea, what is Apple even thinking?
To be sure, have other drivers been moved to user-space?
For all we know they might also be broken. It needs to be checked, it seems doubtful they would only move one driver over.
On Tue, Feb 18, 2025 at 21:00, l0rinc ***@***.***(mailto:On Tue, Feb 18, 2025 at 21:00, l0rinc <<a href=)> wrote:
> @l0rinc commented on this pull request.
...
💬 tnull commented on pull request "Fix delimeter in `package-msg` field of `submitpackage` RPC":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2666838815)
> lgtm, though I've heard we don't like hyphens, so perhaps go in the other direction and change all the other ones to `_`?
Hmm, sure, happy to change if that would be preferable, mostly went this way as I saw other examples using `-` (e.g., `p2sh-segwit` in `decodescript` result, `bit125-replaceable` in the wallet RPCs, etc), but I now realize there are also many (more?) that use `_`. This makes me wonder if we are really positive it's worth breaking the current API, if neither of the option
...
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2666838815)
> lgtm, though I've heard we don't like hyphens, so perhaps go in the other direction and change all the other ones to `_`?
Hmm, sure, happy to change if that would be preferable, mostly went this way as I saw other examples using `-` (e.g., `p2sh-segwit` in `decodescript` result, `bit125-replaceable` in the wallet RPCs, etc), but I now realize there are also many (more?) that use `_`. This makes me wonder if we are really positive it's worth breaking the current API, if neither of the option
...
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960532733)
thanks, removed `DEFAULT_MAX_ORPHAN_TRANSACTIONS`
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960532733)
thanks, removed `DEFAULT_MAX_ORPHAN_TRANSACTIONS`
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960532993)
thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960532993)
thanks, fixed
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960533250)
changed to be asserting exact counts 👍
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960533250)
changed to be asserting exact counts 👍
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2666866811)
@ismaelsadeeq
> It seems to me that this is designed with the flexibility to accommodate more levels, which explains the use of `std::vector` for `ClusterSet`
It's not really written with the intent of accomodating more levels; it's more that I found it more elegant to write it generically, because most operations don't care what level they are operating on. I can see that the increased abstraction may be confusing though; I'd like to hear more reviewer comments on this.
> If yes then
...
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2666866811)
@ismaelsadeeq
> It seems to me that this is designed with the flexibility to accommodate more levels, which explains the use of `std::vector` for `ClusterSet`
It's not really written with the intent of accomodating more levels; it's more that I found it more elegant to write it generically, because most operations don't care what level they are operating on. I can see that the increased abstraction may be confusing though; I'd like to hear more reviewer comments on this.
> If yes then
...
📝 maflcko opened a pull request: " contrib: Add deterministic-unittest-coverage "
(https://github.com/bitcoin/bitcoin/pull/31901)
The `contrib/devtools/test_deterministic_coverage.sh` script is problematic:
* It is written in bash. This can lead to issues when running with the ancient bash version shipped by macOS by default, or can lead to other compatibility issues, such as https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827. Also, pipefail isn't set, so IO errors may be silently ignored.
* It is based on gcov. This can lead to issues, such as https://github.com/bitcoin/bitcoin/pull/31588#pullrequest
...
(https://github.com/bitcoin/bitcoin/pull/31901)
The `contrib/devtools/test_deterministic_coverage.sh` script is problematic:
* It is written in bash. This can lead to issues when running with the ancient bash version shipped by macOS by default, or can lead to other compatibility issues, such as https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827. Also, pipefail isn't set, so IO errors may be silently ignored.
* It is based on gcov. This can lead to issues, such as https://github.com/bitcoin/bitcoin/pull/31588#pullrequest
...
💬 glozow commented on pull request "Fix delimeter in `package-msg` field of `submitpackage` RPC":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2666867794)
I want to say this changed over time... I can't find the thread where I was convinced, but here's chat GPT's argument for why underscores are better than hyphens in JSON:
1. Consistency with JavaScript & JSON standards – JSON keys are commonly accessed in JavaScript, and using hyphens (some-key) requires bracket notation (obj["some-key"]) instead of the simpler dot notation (obj.some_key).
2. Better compatibility – Many programming languages (Python, JavaScript, etc.) use underscores for var
...
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2666867794)
I want to say this changed over time... I can't find the thread where I was convinced, but here's chat GPT's argument for why underscores are better than hyphens in JSON:
1. Consistency with JavaScript & JSON standards – JSON keys are commonly accessed in JavaScript, and using hyphens (some-key) requires bracket notation (obj["some-key"]) instead of the simpler dot notation (obj.some_key).
2. Better compatibility – Many programming languages (Python, JavaScript, etc.) use underscores for var
...
💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2666868256)
Done in https://github.com/bitcoin/bitcoin/pull/31901
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2666868256)
Done in https://github.com/bitcoin/bitcoin/pull/31901
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2666868659)
Done in https://github.com/bitcoin/bitcoin/pull/31901
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2666868659)
Done in https://github.com/bitcoin/bitcoin/pull/31901
🤔 ryanofsky reviewed a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2624615771)
Thanks for the review! Updated with some simplifications and cleanups based on your suggestions.
Updated bfadc7d241a6288e70fb0c58b6ad5fa5a305af51 -> 11615b1025a09e62f12224ae55c92fe85c42b24c ([`pr/wrapp.1`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.1) -> [`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.1..pr/wrapp.2)) adding get_binaries method, renaming some variables, and avoiding `getattr`.
...
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2624615771)
Thanks for the review! Updated with some simplifications and cleanups based on your suggestions.
Updated bfadc7d241a6288e70fb0c58b6ad5fa5a305af51 -> 11615b1025a09e62f12224ae55c92fe85c42b24c ([`pr/wrapp.1`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.1) -> [`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.1..pr/wrapp.2)) adding get_binaries method, renaming some variables, and avoiding `getattr`.
...
💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960517840)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959430288
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): I'd say it would be clearer to not have `self.paths` at all, and also remove the need to somehow import and handle `Binaries` in individual tests.
That's a good idea. Added a `TestFramework.get_binaries()` method to make this more convenient. There are only 3 tests that use it, but it is also convenient to use inter
...
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960517840)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959430288
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): I'd say it would be clearer to not have `self.paths` at all, and also remove the need to somehow import and handle `Binaries` in individual tests.
That's a good idea. Added a `TestFramework.get_binaries()` method to make this more convenient. There are only 3 tests that use it, but it is also convenient to use inter
...
💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960373605)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?
Followup commit 60b89ab4897e175c8d73a5dd21cd52a0c0c790ef from #31375 might provide context because these names just match names of `bitcoin` subcommands.
To answe
...
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960373605)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?
Followup commit 60b89ab4897e175c8d73a5dd21cd52a0c0c790ef from #31375 might provide context because these names just match names of `bitcoin` subcommands.
To answe
...
💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960332653)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?
Imo, plain objects are simpler than dicts, more readable (`o.prop` vs `d["prop"]`) and more compatible with useful features like static type checking.
Right now the `paths` object is just a collection of paths to executa
...
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1960332653)
re: https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105
> nit in [e58f5e0](https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078): Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?
Imo, plain objects are simpler than dicts, more readable (`o.prop` vs `d["prop"]`) and more compatible with useful features like static type checking.
Right now the `paths` object is just a collection of paths to executa
...
💬 ryanofsky commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2666883972)
Updated 11615b1025a09e62f12224ae55c92fe85c42b24c -> 8f5228d9239464ece06f3a56372aeec29685801c ([`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2) -> [`pr/wrapp.3`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.2..pr/wrapp.3)) to fix lint errors
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2666883972)
Updated 11615b1025a09e62f12224ae55c92fe85c42b24c -> 8f5228d9239464ece06f3a56372aeec29685801c ([`pr/wrapp.2`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.2) -> [`pr/wrapp.3`](https://github.com/ryanofsky/bitcoin/commits/pr/wrapp.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrapp.2..pr/wrapp.3)) to fix lint errors
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1960567077)
It might still do something; for example removals may be applied still, and grouping may be calculated. I feel it's unnecessary to elaborate that much about implementation details in the interface, though.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1960567077)
It might still do something; for example removals may be applied still, and grouping may be calculated. I feel it's unnecessary to elaborate that much about implementation details in the interface, though.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960567794)
Maybe I'm misunderstanding, but I don't think we can use `RemoveIterAt` (which is for updating the peer list) for this (which is the global `TxOrphanage::m_orphan_list`)?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960567794)
Maybe I'm misunderstanding, but I don't think we can use `RemoveIterAt` (which is for updating the peer list) for this (which is the global `TxOrphanage::m_orphan_list`)?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960570915)
Oh of course; it just looked very similar.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960570915)
Oh of course; it just looked very similar.
💬 s373nZ commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2666905054)
> I don't understand the motivation for setting `CMAKE_<lang>_COMPILER_LAUNCHER` from inside the project.
I'm also ignorantly curious about this. Is it primarily a requirement from the feature parity list with the prior `autotools`?
https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2666905054)
> I don't understand the motivation for setting `CMAKE_<lang>_COMPILER_LAUNCHER` from inside the project.
I'm also ignorantly curious about this. Is it primarily a requirement from the feature parity list with the prior `autotools`?
https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960581755)
thanks, done
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960581755)
thanks, done