💬 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.
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021053788)
You should be able to test my claim by panicking the threads and observing that the `Err` never makes it out.
See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b7091d91fc333d2e260327c46a90dec6
Of course, this is fine here. I just wanted to explain the change in behavior here and why I choose the more consistent behavior myself here.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021053788)
You should be able to test my claim by panicking the threads and observing that the `Err` never makes it out.
See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b7091d91fc333d2e260327c46a90dec6
Of course, this is fine here. I just wanted to explain the change in behavior here and why I choose the more consistent behavior myself here.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021054952)
> if you want less libfuzzer output, it would be easier to compile without libfuzzer
Given the defaults in contrib/devtools/README.md I still think this would be a good change.
Missed that `output()` still allowed checking the exit code. You are right in that it cleaned things up: a7f4e4b7cd0a04ae7c84ad7afa284006a600e369
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021054952)
> if you want less libfuzzer output, it would be easier to compile without libfuzzer
Given the defaults in contrib/devtools/README.md I still think this would be a good change.
Missed that `output()` still allowed checking the exit code. You are right in that it cleaned things up: a7f4e4b7cd0a04ae7c84ad7afa284006a600e369
👋 glozow's pull request is ready for review: "[29.x] backports and 29.0rc3"
(https://github.com/bitcoin/bitcoin/pull/32136)
(https://github.com/bitcoin/bitcoin/pull/32136)
🤔 fjahr reviewed a pull request: "wallet, migration: Fix empty wallet crash"
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2729324019)
tACK d4cb6d048b0222cf51c8a56fa56f97dd5d2c9add
Changes look good to me and I confirmed that the new test fails on master.
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2729324019)
tACK d4cb6d048b0222cf51c8a56fa56f97dd5d2c9add
Changes look good to me and I confirmed that the new test fails on master.
💬 fanquake commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766324142)
Yea, it's kind of ridiculous the amount of packages we need to build and cache for the native Windows build (and the time it takes for a full compile). This is also one of the most common causes of sporadic CI failures, when dependencies of dependencies (which we don't even use, i.e a compression library compiled as a qt input) fail for some sporadic reason.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766324142)
Yea, it's kind of ridiculous the amount of packages we need to build and cache for the native Windows build (and the time it takes for a full compile). This is also one of the most common causes of sporadic CI failures, when dependencies of dependencies (which we don't even use, i.e a compression library compiled as a qt input) fail for some sporadic reason.
🚀 fanquake merged a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)
(https://github.com/bitcoin/bitcoin/pull/31176)
💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021101693)
> So maybe it's worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it's out of scope for this PR.
i'll open a PR for this.
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021101693)
> So maybe it's worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it's out of scope for this PR.
i'll open a PR for this.