💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055602303)
If there was some kind of `UnreachableError` I would use it. Looked into using our utility `assert_raises(...stop_node...)`, but that swallows the exception, so the test fails.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055602303)
If there was some kind of `UnreachableError` I would use it. Looked into using our utility `assert_raises(...stop_node...)`, but that swallows the exception, so the test fails.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055653426)
The full example from the python docs is:
```python
try:
f = open('myfile.txt')
s = f.readline()
i = int(s.strip())
except OSError as err:
print("OS error:", err)
except ValueError:
print("Could not convert data to an integer.")
except Exception as err:
print(f"Unexpected {err=}, {type(err)=}")
raise
```
So the first and last lines inside the `try` are expected to sometimes raise exceptions (maybe the `readline()` too), which does not map well to the cod
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055653426)
The full example from the python docs is:
```python
try:
f = open('myfile.txt')
s = f.readline()
i = int(s.strip())
except OSError as err:
print("OS error:", err)
except ValueError:
print("Could not convert data to an integer.")
except Exception as err:
print(f"Unexpected {err=}, {type(err)=}")
raise
```
So the first and last lines inside the `try` are expected to sometimes raise exceptions (maybe the `readline()` too), which does not map well to the cod
...
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055595632)
Pushed an alternative, still using `print`. When I tried `sys.stderr.buffer.write` that data ended up at the end of the failure output, instead of when the exception was caught.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055595632)
Pushed an alternative, still using `print`. When I tried `sys.stderr.buffer.write` that data ended up at the end of the failure output, instead of when the exception was caught.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055664123)
The point is that the happy path is separated from the error handling, we're not using exceptions as if they were returned error codes
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055664123)
The point is that the happy path is separated from the error handling, we're not using exceptions as if they were returned error codes
💬 maflcko commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823701217)
> Under the `DEBUG` build type it takes ~140s, the next longest running test is ~80s; so I imagine some devs will wonder why runing ctest is now taking > 2-3 minutes to complete.
I wonder if this is relevant, given how long the functional tests take. And if someone doesn't want to run all tests, they can trivially exclude all benchmarks via `ctest --exclude-regex ...`.
An alternative may be to allow running the individual benchmarks in parallel, similar to the unit tests.
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823701217)
> Under the `DEBUG` build type it takes ~140s, the next longest running test is ~80s; so I imagine some devs will wonder why runing ctest is now taking > 2-3 minutes to complete.
I wonder if this is relevant, given how long the functional tests take. And if someone doesn't want to run all tests, they can trivially exclude all benchmarks via `ctest --exclude-regex ...`.
An alternative may be to allow running the individual benchmarks in parallel, similar to the unit tests.
🤔 janb84 reviewed a pull request: "depends: Fix cross-compiling on macOS"
(https://github.com/bitcoin/bitcoin/pull/32215#pullrequestreview-2786676891)
ACK [d0cce41](https://github.com/bitcoin/bitcoin/pull/32215/commits/d0cce4172c041fc9b2910b360fe496b1102b19d2)
Recreated issue #32208 on master, confirmed it's solved after this PR.
(https://github.com/bitcoin/bitcoin/pull/32215#pullrequestreview-2786676891)
ACK [d0cce41](https://github.com/bitcoin/bitcoin/pull/32215/commits/d0cce4172c041fc9b2910b360fe496b1102b19d2)
Recreated issue #32208 on master, confirmed it's solved after this PR.
💬 fanquake commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823725523)
> I wonder if this is relevant, given how long the functional tests take.
My assumption would be that developers are running the units tests regularly, far more often than they are running the entire functional test suite.
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823725523)
> I wonder if this is relevant, given how long the functional tests take.
My assumption would be that developers are running the units tests regularly, far more often than they are running the entire functional test suite.
💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2823736638)
Tested 86c7c65e2fe7a33f47e08d7c344dbee44ea1e379 on Windows 11 against HWI 3.1.0 on testnet4.
First time it crashes immediately after creating a new wallet from a Trezor model T. The wallet ended up blank with no descriptors. The debug log shows nothing useful, except that it didn't import descriptors. cc @achow101
```
2025-04-23T09:50:08Z Using SQLite Version 3.46.1
2025-04-23T09:50:08Z Using wallet C:\Users\sjors\AppData\Roaming\Bitcoin\testnet4\wallets\TrezorT
2025-04-23T09:50:08Z in
...
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2823736638)
Tested 86c7c65e2fe7a33f47e08d7c344dbee44ea1e379 on Windows 11 against HWI 3.1.0 on testnet4.
First time it crashes immediately after creating a new wallet from a Trezor model T. The wallet ended up blank with no descriptors. The debug log shows nothing useful, except that it didn't import descriptors. cc @achow101
```
2025-04-23T09:50:08Z Using SQLite Version 3.46.1
2025-04-23T09:50:08Z Using wallet C:\Users\sjors\AppData\Roaming\Bitcoin\testnet4\wallets\TrezorT
2025-04-23T09:50:08Z in
...
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055688076)
My intent is to keep as much of the code separate from error handling as possible. By narrowing the scope of the try it makes clear to the reader that there aren't exceptions hiding in every line of the happy path.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055688076)
My intent is to keep as much of the code separate from error handling as possible. By narrowing the scope of the try it makes clear to the reader that there aren't exceptions hiding in every line of the happy path.
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055693705)
Fixed. @TheCharlatan why was this set to `Yes` in the original?
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055693705)
Fixed. @TheCharlatan why was this set to `Yes` in the original?
💬 maflcko commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055699796)
depedency → dependency
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055699796)
depedency → dependency
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2823758637)
An alternative approach could be to make Cap'n Proto optional and have `-DENABLE_IPC` default to `ON` only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2823758637)
An alternative approach could be to make Cap'n Proto optional and have `-DENABLE_IPC` default to `ON` only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2823759748)
ACK fa19d8bee350fef088f5fabca38f09610fa8d20d
The rebase had to fix a `Proxy` merge conflict but otherwise is a clean cherry-pick of the previous `readability-avoid-const-params-in-decls` commit.
Q: Any reason for not including these `const` removals in the last commit?
* `interfaces::BlockTemplate::waitNext`
* `node::GetMinimumTime`
* `blockToJSON`
* ` blockheaderToJSON`
* `GetTarget`
* `wallet::CreateDescriptor`
* `wallet::CWallet::ListAddrBookFunc`
* `DeriveTarget
...
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2823759748)
ACK fa19d8bee350fef088f5fabca38f09610fa8d20d
The rebase had to fix a `Proxy` merge conflict but otherwise is a clean cherry-pick of the previous `readability-avoid-const-params-in-decls` commit.
Q: Any reason for not including these `const` removals in the last commit?
* `interfaces::BlockTemplate::waitNext`
* `node::GetMinimumTime`
* `blockToJSON`
* ` blockheaderToJSON`
* `GetTarget`
* `wallet::CreateDescriptor`
* `wallet::CWallet::ListAddrBookFunc`
* `DeriveTarget
...
💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942)
Specficially → Specifically
Also, `bitcoin*d*-wallet` seems odd.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942)
Specficially → Specifically
Also, `bitcoin*d*-wallet` seems odd.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055708651)
I think the opposite is true, this way we're forcing the reader to consider the error case when trying to understand the happy path (i.e. separation of concerns). But I'll leave it, it's not *that* important.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055708651)
I think the opposite is true, this way we're forcing the reader to consider the error case when trying to understand the happy path (i.e. separation of concerns). But I'll leave it, it's not *that* important.
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055699711)
In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055699711)
In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055701332)
In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055701332)
In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055708434)
In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn't mention what they are, or explain why we aren't fixing the code?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055708434)
In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn't mention what they are, or explain why we aren't fixing the code?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055703963)
In ce950ec20fa0d246d2fd44f562a864d3ff83d399 "Fix quote issue on Windows ": This isn't in a Windows block, so I'm guessing it doesn't have any effect on other OS's? Is this alos going to be upstreamed?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055703963)
In ce950ec20fa0d246d2fd44f562a864d3ff83d399 "Fix quote issue on Windows ": This isn't in a Windows block, so I'm guessing it doesn't have any effect on other OS's? Is this alos going to be upstreamed?
💬 0xB10C commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2823781979)
Seems like a one-off event for now and not much that can be done about it?
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2823781979)
Seems like a one-off event for now and not much that can be done about it?