💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2055412943)
Alright, I agree `OutboundConnectedToStr()` is a bit odd. If it is renamed to `OutboundConnectedToAddress()`, then what about `OutboundConnectedToService()`? Leave it like this? Then it would be confusing because we have the `CAddress` type and the `...Address()` suffix would imply such an argument. I wanted to avoid overload (two different methods with the same name) because that is not grep-friendly. Should I drop that and have an overload?

Maybe:
* `ConnectedToAddrPort(string)` and `Conne
...

💬 janb84 commented on pull request "depends: Fix cross-compiling on macOS":
(https://github.com/bitcoin/bitcoin/pull/32215#issuecomment-2823422336)
> @janb84
>
> > ... but I do get an different error back
>
> Please provide more details about your build environment (OS version, architecture, compiler version etc) and exact steps to reproduce the error.

Of course, here you go, ill will also try to figure it out also but please let me know if you need any other details.

Build env:
MacOS 15.4.1, M1
QT 6.9.0
Tried both Apple clang version 17.0.0 (clang-1700.0.13.3) & Homebrew clang version 20.1.3
SDK 15.4

Steps to reprodu
...

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor (if `internal=false`) & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2823457474)
PR description (and title) updated

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2823457894)
Rebased after #32293, updated the documentation it introduced, and added Cap'n Proto to `doc/dependencies.md`. Also took the LLM fix.

💬 stratospher commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2041644069)
9c4aad2a: could remove `strprintf`

💬 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.

💬 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
...

💬 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.

💬 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

💬 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 |
```

💬 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?

👍 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:

💬 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
...

🚀 fanquake merged a pull request: "test: Run all benchmarks in the sanity check"
(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.

💬 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
...

💬 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.

💬 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

💬 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.

🤔 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.