💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020926628)
Since you're touching this line... According to the internet, we should also check `EWOULDBLOCK` even though it's usually the same as `EAGAIN`, and it's likely not relevant for any system we support.
https://stackoverflow.com/a/49421517
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020926628)
Since you're touching this line... According to the internet, we should also check `EWOULDBLOCK` even though it's usually the same as `EAGAIN`, and it's likely not relevant for any system we support.
https://stackoverflow.com/a/49421517
💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020940099)
I know this is existing code, but I don't recall why there's no timeout here. And also, should there be a quick wait between `Recv` calls?
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020940099)
I know this is existing code, but I don't recall why there's no timeout here. And also, should there be a quick wait between `Recv` calls?
💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020953743)
> checking of the sequence number on each message to ensure it was meant for our request
This seems like a good idea to at least do in debug builds.
It seems like a good precaution to check for the presence of `NLM_F_MULTI` and don't wait for `NLMSG_DONE` if it isn't. At least from my naive reading of https://man7.org/linux/man-pages/man7/netlink.7.html it seems `NLMSG_DONE` is only used for multipart messages.
Splitting into multiple commits would be useful, e.g. one commit that switch
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020953743)
> checking of the sequence number on each message to ensure it was meant for our request
This seems like a good idea to at least do in debug builds.
It seems like a good precaution to check for the presence of `NLM_F_MULTI` and don't wait for `NLMSG_DONE` if it isn't. At least from my naive reading of https://man7.org/linux/man-pages/man7/netlink.7.html it seems `NLMSG_DONE` is only used for multipart messages.
Splitting into multiple commits would be useful, e.g. one commit that switch
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020970786)
I agree that the depends caching strategy could be optimized in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020970786)
I agree that the depends caching strategy could be optimized in a follow-up.
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971496)
Thx, will fixup, if I have to re-touch
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971496)
Thx, will fixup, if I have to re-touch
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971596)
Not sure. If someone wants to fuzz test the script worker threads, a dedicated fuzz target seems ideal (See `./src/test/fuzz/checkqueue.cpp`). Relying on unrelated fuzz targets to achieve coverage here seems brittle at best, because those targets may change at any time, dropping the coverage silently. Also, I am not aware of any meaningful coverage here in any fuzz target that would be more than what the unit tests and functional tests already achieve.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971596)
Not sure. If someone wants to fuzz test the script worker threads, a dedicated fuzz target seems ideal (See `./src/test/fuzz/checkqueue.cpp`). Relying on unrelated fuzz targets to achieve coverage here seems brittle at best, because those targets may change at any time, dropping the coverage silently. Also, I am not aware of any meaningful coverage here in any fuzz target that would be more than what the unit tests and functional tests already achieve.
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971674)
I don't think your suggestion works. In Rust, the `?` operator is the early-return operator. This means, if you use it in a while loop that joins threads, it may return early and leave some threads dangling.
This is not much of an issue here, as `thread::scope` takes care of joining them, once they go out of scope. However, the main thread may now sometimes return an Err, or panic, when a child panics. This seems confusing. Also, if in the future all errors want to be collected and returned,
...
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971674)
I don't think your suggestion works. In Rust, the `?` operator is the early-return operator. This means, if you use it in a while loop that joins threads, it may return early and leave some threads dangling.
This is not much of an issue here, as `thread::scope` takes care of joining them, once they go out of scope. However, the main thread may now sometimes return an Err, or panic, when a child panics. This seems confusing. Also, if in the future all errors want to be collected and returned,
...
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020972495)
Seems fine, but I'd say, if you want less libfuzzer output, it would be easier to compile without libfuzzer, especially given that it is not needed for this tool.
Also, code-wise, it would probably be easier to just use https://doc.rust-lang.org/std/process/struct.Command.html#method.output instead of spawning and handling a child manually.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020972495)
Seems fine, but I'd say, if you want less libfuzzer output, it would be easier to compile without libfuzzer, especially given that it is not needed for this tool.
Also, code-wise, it would probably be easier to just use https://doc.rust-lang.org/std/process/struct.Command.html#method.output instead of spawning and handling a child manually.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766134326)
@ryanofsky Do you think this is rfm?
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766134326)
@ryanofsky Do you think this is rfm?
👍 hodlinator approved a pull request: "torcontrol: Define tor reply code as const to improve our maintainability"
(https://github.com/bitcoin/bitcoin/pull/32166#pullrequestreview-2729116074)
crACK f31ce35966bb84608938b0ba2272b415bcd42618
Could move "Friendly invite ..." out of PR description into its own comment.
(https://github.com/bitcoin/bitcoin/pull/32166#pullrequestreview-2729116074)
crACK f31ce35966bb84608938b0ba2272b415bcd42618
Could move "Friendly invite ..." out of PR description into its own comment.
💬 hodlinator commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020944180)
nit: Others might disagree but I prefer grouping with an enum:
```suggestion
enum {
TOR_REPLY_OK = 250,
TOR_REPLY_UNRECOGNIZED = 510,
};
```
(Developer-notes prefer `enum class` but I think implicit conversion to `int` is more important in this case, and there is no risk of forgetting the non-existent enum name).
These constants are hardcoded in src/test/fuzz/torcontrol.cpp as well, but I think it's okay to let that be.
If you decide to not convert them to `enum`, it would b
...
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020944180)
nit: Others might disagree but I prefer grouping with an enum:
```suggestion
enum {
TOR_REPLY_OK = 250,
TOR_REPLY_UNRECOGNIZED = 510,
};
```
(Developer-notes prefer `enum class` but I think implicit conversion to `int` is more important in this case, and there is no risk of forgetting the non-existent enum name).
These constants are hardcoded in src/test/fuzz/torcontrol.cpp as well, but I think it's okay to let that be.
If you decide to not convert them to `enum`, it would b
...
💬 hodlinator commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021002729)
nit: Think clang-tidy will verify the name matches if you append `=` to the comment.
```suggestion
Proxy addrOnion = Proxy(resolved, /*_randomize_credentials=*/ true);
```
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021002729)
nit: Think clang-tidy will verify the name matches if you append `=` to the comment.
```suggestion
Proxy addrOnion = Proxy(resolved, /*_randomize_credentials=*/ true);
```
💬 hodlinator commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020973352)
informational, related:
Still feels scary to me how `_randomize_credentials=true` leads to a predictably incrementing integer to be used for user/pass after 5eaaa83ac1f5eb525f93e2808fafd73f5ed97013. What if guarantees of the Tor API were misunderstood, or change in the future?
Guess it's very unlikely to be a problem if these are only used to authenticate with the local SOCKS5 proxy and don't reach any further.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020973352)
informational, related:
Still feels scary to me how `_randomize_credentials=true` leads to a predictably incrementing integer to be used for user/pass after 5eaaa83ac1f5eb525f93e2808fafd73f5ed97013. What if guarantees of the Tor API were misunderstood, or change in the future?
Guess it's very unlikely to be a problem if these are only used to authenticate with the local SOCKS5 proxy and don't reach any further.
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766164288)
> These sizes look quite acceptable.
To clarify my calculation: The `win64-native-vcpkg-binary` cache takes the longest to compute, because the windows task is the slowest. So if there are three merges, where two of them modify the `win64-native-vcpkg-binary` cache key, there will be three trailing writes of the cache of a size of 2.6GB each. This will consume ~7.8GB, so after one more task is added after this one here, the cache could start breaking/evicting non-windows caches with two pus
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766164288)
> These sizes look quite acceptable.
To clarify my calculation: The `win64-native-vcpkg-binary` cache takes the longest to compute, because the windows task is the slowest. So if there are three merges, where two of them modify the `win64-native-vcpkg-binary` cache key, there will be three trailing writes of the cache of a size of 2.6GB each. This will consume ~7.8GB, so after one more task is added after this one here, the cache could start breaking/evicting non-windows caches with two pus
...
💬 eval-exec commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021024160)
Thank you.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021024160)
Thank you.
💬 eval-exec commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021027893)
I'm fine with either option; let the Bitcoin maintainers decide.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021027893)
I'm fine with either option; let the Bitcoin maintainers decide.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021028255)
> I don't think your suggestion works.
I've verified it works by executing something that fails determinism:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz ../qa-assets/fuzz_corpora/ connman 16
```
> it may return early and leave some threads dangling.
https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html states:
"An owned permission to join on a scoped thread (block on its termination)."
> However,
...
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021028255)
> I don't think your suggestion works.
I've verified it works by executing something that fails determinism:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz ../qa-assets/fuzz_corpora/ connman 16
```
> it may return early and leave some threads dangling.
https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html states:
"An owned permission to join on a scoped thread (block on its termination)."
> However,
...
💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021044241)
The requirements for stream isolation are documented here: https://spec.torproject.org/socks-extensions.html#stream-isolation . Our case is "They are both SOCKS5 with USERNAME/PASSWORD authentication, using legacy isolation parameters, and they have identical usernames and identical passwords". The `<torS0X>` extension is very recent so we don't use that (yet?).
So using different credentials is enough, there is no requirement, nor even benefit for them to be random.
Although thinking of i
...
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021044241)
The requirements for stream isolation are documented here: https://spec.torproject.org/socks-extensions.html#stream-isolation . Our case is "They are both SOCKS5 with USERNAME/PASSWORD authentication, using legacy isolation parameters, and they have identical usernames and identical passwords". The `<torS0X>` extension is very recent so we don't use that (yet?).
So using different credentials is enough, there is no requirement, nor even benefit for them to be random.
Although thinking of i
...
💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021046837)
Pretty sure it is was already validated, this is how we discovered that the parameter is `_randomize_credentials` not `randomize_credentials`.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021046837)
Pretty sure it is was already validated, this is how we discovered that the parameter is `_randomize_credentials` not `randomize_credentials`.
💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021049987)
Oh, we used to have a `=` but it was lost? You're right.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021049987)
Oh, we used to have a `=` but it was lost? You're right.