💬 stratospher commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2041903792)
2acd12c: micro nit: s/trough/through.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2041903792)
2acd12c: micro nit: s/trough/through.
💬 stratospher commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2047059613)
655af59:
> If the malleated transaction wins then we will eventually stop broadcasting the original when it gets stale and gets removed from the "to broadcast" storage cause it is not acceptable in our mempool." doesn't happen.
looks like `Remove(stale_tx)` doesn't happen here (returns `nullopt` without removing the transaction from `m_by_txid`) and we keep re-attempting private broadcast of this malleated transaction scenario every 10-15 mins.
constructing a malleated transaction wasn't
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2047059613)
655af59:
> If the malleated transaction wins then we will eventually stop broadcasting the original when it gets stale and gets removed from the "to broadcast" storage cause it is not acceptable in our mempool." doesn't happen.
looks like `Remove(stale_tx)` doesn't happen here (returns `nullopt` without removing the transaction from `m_by_txid`) and we keep re-attempting private broadcast of this malleated transaction scenario every 10-15 mins.
constructing a malleated transaction wasn't
...
💬 stratospher commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2041693437)
4e45e9c: micro nit: could we also mention "transaction" here and in `src/rpc/net.cpp`? people looking at help might be confused about what privacy-sensitive data is meant here.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2041693437)
4e45e9c: micro nit: could we also mention "transaction" here and in `src/rpc/net.cpp`? people looking at help might be confused about what privacy-sensitive data is meant here.
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2055546342)
In ec5d2c7fca5e6c36aa59aeb7ce64b98739b6d094 "wallet: Disallow legacy wallet creation from the wallet tool": I'm getting a trailing `%` on macOS
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2055546342)
In ec5d2c7fca5e6c36aa59aeb7ce64b98739b6d094 "wallet: Disallow legacy wallet creation from the wallet tool": I'm getting a trailing `%` on macOS
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055580572)
```suggestion
| [Cap'n Proto](../depends/packages/capnp.mk) | [link](https://capnproto.org) | [0.6.1](https://capnproto.org/install.html) | 0.5.3 | No |
```
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055580572)
```suggestion
| [Cap'n Proto](../depends/packages/capnp.mk) | [link](https://capnproto.org) | [0.6.1](https://capnproto.org/install.html) | 0.5.3 | No |
```
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2055590028)
Just sorted the imports for consistency - but I see that the corresponding header is sometimes separated, so I've reverted this move.
What do you think about the whole change in general, can you give me a conceptual review?
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2055590028)
Just sorted the imports for consistency - but I see that the corresponding header is sometimes separated, so I've reverted this move.
What do you think about the whole change in general, can you give me a conceptual review?
👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2786509505)
ACK 81c0b9edfe533afbb2f4dda56142afdedffdb347
> @vasild I think you got the wrong hash.
Yes, that was `HEAD~` at the time :space_invader:
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2786509505)
ACK 81c0b9edfe533afbb2f4dda56142afdedffdb347
> @vasild I think you got the wrong hash.
Yes, that was `HEAD~` at the time :space_invader:
💬 fanquake commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823620332)
Locally the sanity bench (when running `ctest`) is taking ~19s, roughly the same time as the second worst secp256k1 test. 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 checked that under Valgrind the runtime isn't so long that it'd cause the job to fail due to timeout (although there might be a different issue causing that).
I think ideally we could just delete
...
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823620332)
Locally the sanity bench (when running `ctest`) is taking ~19s, roughly the same time as the second worst secp256k1 test. 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 checked that under Valgrind the runtime isn't so long that it'd cause the job to fail due to timeout (although there might be a different issue causing that).
I think ideally we could just delete
...
🚀 fanquake merged a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310)
(https://github.com/bitcoin/bitcoin/pull/32310)
💬 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