💬 hebasto commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487242811)
The GHA [docs](https://docs.github.com/en/actions/reference/workflows-and-actions/metadata-syntax) do not mention `inputs.<input_id>.options` at all.

👍 hebasto approved a pull request: "random: scope environ extern to macOS, BSDs and Illumos"
(https://github.com/bitcoin/bitcoin/pull/33714#pullrequestreview-3412082427)
re-ACK 79d6f458e2300e1f47b94467cda233e1c761f8be.

💬 willcl-ark commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487261226)
Yep, I think the original implementation got wires crossed with workflow dispatch inputs https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-of-onworkflow_dispatchinputs

💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2487266295)
This edit is a tautology. No matter what the change budgeting is, the change amount will be at least `CHANGE_LOWER` since this value is added to `CHANGE_LOWER`. Maybe it's not the place to explain to users of the API _why_ it matters that `CHANGE_LOWER` may not be enough of a change budget, although I would have no idea why without reading the commit that added this parameter.

💬 hebasto commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3481668679)
> Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc [@hebasto](https://github.com/hebasto)

cc @GBKS @johnny9

💬 fanquake commented on pull request "feat: shell completions for bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/33747#issuecomment-3481668738)
Thanks. However aside from the fish addition, this duplicates works already being done in #33385 and #33402.

💬 purpleKarrot commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487296852)
Thanks @stickies-v.

I am not convinced about `std::strong_ordering`.
`constexpr` will definitely be added as a follow up, together with `= default` where appropriate.

💬 fanquake commented on pull request "depends: disable variables, rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3481696397)
Guix Build (aarch64):
```bash
e0edc03339b5b32705b8eacbc213a6fd6e5b787caba574671bb3c18340a141ec guix-build-52b1595850f6/output/aarch64-linux-gnu/SHA256SUMS.part
55778a17e64cabdb200679240fa004b5c5c9a684ecae773e8497997ab7c968d6 guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu-debug.tar.gz
6a2f49dbc8238438d40d46c3ce699814ae1d9eb6d74dd0b4ff273db6648b181c guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu.tar.gz
2fce92
...

💬 johnny9 commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3481702959)
> > Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc [@hebasto](https://github.com/hebasto)
>
> cc [@GBKS](https://github.com/GBKS) [@johnny9](https://github.com/johnny9)

My vote would be to continue to add features to the Qt Widgets gui. There might be differing opinions but I see the two as potentially serving different target platforms as well as different target users.

💬 waketraindev commented on pull request "qt: add console commands for clearing output and history":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3481744961)
Added release notes file.

🤔 hebasto reviewed a pull request: "Increase tooltip wrap threshold from 80 to 100 characters"
(https://github.com/bitcoin-core/gui/pull/905#pullrequestreview-3412212034)
This code was introduced in https://github.com/bitcoin/bitcoin/pull/1090. During that discussion, there was a [concern](https://github.com/bitcoin/bitcoin/pull/1090#issuecomment-5452893) that "tooltip windows [were] too wide sometimes".

This isn’t about fitting on the screen, but about improving readability.

cc @laanwj @GBKS

💬 hulxv commented on pull request "feat: shell completions for bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/33747#issuecomment-3481770602)
> Thanks. However aside from the fish addition, this duplicates works already being done in #33385 and #33402.

Actually, bash is the only is implemented in #33385 but zsh isn't implemented in #33402

Also in #33385, he covers only `rpc`, `node`, and `tx`, but mine covers all subcommands

🤔 sipa reviewed a pull request: "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-3412258624)
ACK 6bf4f4a87889b0679c744b0a171c98a9386fe8c0

Just one test-only comment.

💬 sipa commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2487390409)
In commit "refactor(miniscript): Remove superfluous unique_ptr-indirection"

Should this be `const std::optional<Node>& node` (by reference)?

💬 sipa commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-3481865789)
Code review ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2

Conceptually seems fine too, but I'd want to see @achow101's opinion on that.

🤔 sipa reviewed a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-3412340683)
utACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3482056374)
`a9d697c0e1...c6f46943d1`: rebase due to conflicts, https://github.com/bitcoin/bitcoin/pull/33567 was merged so removed that change from here. Thanks!

💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3482056998)
> Are you still working on this?

Yes am still working on this
Caught up on something else

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3482164060)
Tested running the Linux->Windows CI locally and this patch seems to fix the failure:
```diff
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.

struct HeadersGeneratorSetup : public RegTestingSetup {
const CBlock& genesis{Params().GenesisBlock()};
- const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.L
...

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3482304222)
> It might be good to align the behaviour of feeler connections made over private networks with the private relay logic. ie, rather than introducing a new connection type, overload feeler connections as follows:

There are downsides to this:
- feeler connections are made to random entries from the new table of addrman (which could be flooded with fake spam addrs, making most of the connection attempts fail). Private broadcast txns are chosen like normal connections, with 50% new/tried chance.
...