💬 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.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021106990)
Thanks for the correction.
I believe this would work, even though rustfmt makes the line count less of a win: 67e6d9ece87606297865761f0fb222db9604c180
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021106990)
Thanks for the correction.
I believe this would work, even though rustfmt makes the line count less of a win: 67e6d9ece87606297865761f0fb222db9604c180
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021098980)
Now run through rustfmt: 142e65da352e77f061da3142bff8e2b19e56fc86
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021098980)
Now run through rustfmt: 142e65da352e77f061da3142bff8e2b19e56fc86
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2021114298)
extra ; at the end.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2021114298)
extra ; at the end.
💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2021118543)
https://github.com/bitcoin-core/libmultiprocess/pull/168 renders this line unnecessary.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2021118543)
https://github.com/bitcoin-core/libmultiprocess/pull/168 renders this line unnecessary.
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2021126219)
argh! Thanks!
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2021126219)
argh! Thanks!
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2766373715)
Fixed a typo.
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2766373715)
Fixed a typo.
🤔 furszy reviewed a pull request: "wallet, migration: Fix empty wallet crash"
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2729454159)
Code review ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2729454159)
Code review ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2766402044)
re-ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2766402044)
re-ACK eb4333c8aa4a8f1a66ecbe5c48dbe15467332864
💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2021181809)
Yes, this is a bit more than what I originally intended but I find documenting the relationship b/w the callables here helpful here instead of having to navigate it again when I look at this test after a while.
```diff
- # Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition
- add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_encodeable_pushdata1", key=secs[0], standard=False, failure=
...
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2021181809)
Yes, this is a bit more than what I originally intended but I find documenting the relationship b/w the callables here helpful here instead of having to navigate it again when I look at this test after a while.
```diff
- # Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition
- add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_encodeable_pushdata1", key=secs[0], standard=False, failure=
...