💬 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
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582140)
good point, I've made it a `TxOrphanage` method now
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582140)
good point, I've made it a `TxOrphanage` method now
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582312)
yes, moved it up
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1960582312)
yes, moved it up
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666920246)
> If it's just (very) slow, that may be due to low I/O speed of your disk
Disagree. The alleged cause is not supported by the data:
%iowait = 30.51%
kB_read = 21 624 950
kB_wrtn = 10 959 841
Only 31 blocks (~120 MB) downloaded during the time (c.a. 90 minutes).
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666920246)
> If it's just (very) slow, that may be due to low I/O speed of your disk
Disagree. The alleged cause is not supported by the data:
%iowait = 30.51%
kB_read = 21 624 950
kB_wrtn = 10 959 841
Only 31 blocks (~120 MB) downloaded during the time (c.a. 90 minutes).
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666927835)
> Does it happen on a different filesystem than exfat? Does it happen on a different filesystem hardware, maybe something that is faster? (Can you benchmark the hardware?)
Yes, the same occurs in the same environment except NTFS instead of exFAT and HDD instead of SD.
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666927835)
> Does it happen on a different filesystem than exfat? Does it happen on a different filesystem hardware, maybe something that is faster? (Can you benchmark the hardware?)
Yes, the same occurs in the same environment except NTFS instead of exFAT and HDD instead of SD.
👍 ryanofsky approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2625043669)
Code review ACK fa3fb3c23fae287b161112b9b04cf0937a1051c6. Nice refactoring to get rid of pointless complexity and repetition in this code.
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2625043669)
Code review ACK fa3fb3c23fae287b161112b9b04cf0937a1051c6. Nice refactoring to get rid of pointless complexity and repetition in this code.