💬 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
💬 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).