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

💬 sipa commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3482378717)
Concept ACK. I think deployment of v2-capable nodes is sufficient that it's reasonable to offer an outbound-v2-only option.

💬 sipa commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-3482381863)
Concept ACK

👍 theStack approved a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3412795203)
Concept and code-review ACK fa2fd0ba1fbd4085df24a2c5472636957db28521

Didn't test on Windows. Regarding the first commit, I was wondering why supporting locale-dependent bash scripts was ever relevant, and it seems this was due to a workaround for a segfault-causing bug in `shellcheck` back then (see https://github.com/bitcoin/bitcoin/pull/13454#issuecomment-396968264 ff., in case other reviewers wonder as well).

💬 caesrcd commented on pull request "contrib: shell completions for bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/33747#issuecomment-3482475625)
The bash completion in #33385 is being created in the same way the `bitcoin` command was — as a wrapper. For that reason, since bash completions currently exist only for `bitcoin-cli`, `bitcoind`, and `bitcoin-tx`, it only covers `rpc`, `node`, and `tx`.

I think it’s unnecessary to duplicate existing bash completions. Once #33385 gets merged, we can just add completions for the commands that don’t have them yet.

💬 ajtowns commented on pull request "Fix BIP143 standardness rules for CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33759#issuecomment-3482564006)
Concept NACK. This doesn't seem like a productive use of anyone's time? "passed to a sigop" is at most ambiguous, and that ambiguity is already resolved by the implementation; if it's a problem, update the BIP text to match the implementation. Changing the implementation risks "soft" confiscating funds (ie, transactions that were relayable become non-standard and can only be mined directly), which doesn't seem at all worth doing.

💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3482566467)
> The problem is that PR #5247 doesn't _just_ ban the use of hybrid keys. The implementation goes _way_ overboard and disallows all sorts of data passed to CHECKMULTISIG that isn't even hybrid keys.

I think the process was:

* hybrid keys were banned as a result of:
* discussion on the list at https://gnusha.org/pi/bitcoindev/20120616192651.GA13438@vps7135.xlshosting.net/t/#u
* implemented via #1742 (test only changes)
* activated by #1677 (see https://github.com/bitcoin/bitcoi
...