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

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2487912864)
Move-constructed `std::optional` (which I think would happen here) should be negligible, but agree const ref is more optimal.

Taken in latest push (together with minor correction to another commit's message).

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487919199)
Formatting them to be on top of each other as the above example shows allows easier bit comparison, otherwise the binary representation doesn't really help

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3482711385)
> do you have a proof-of-concept for the tx indexer that will utilize this?

Yes - I am working on a PR to adapt `bindex` to use the new REST API endpoint.
Assuming warm OS block cache, https://github.com/romanz/bindex-rs/pull/63#issuecomment-3448841256 achieves retrieval rate of ~10k txs/second when querying a "popular" signet address.

Will test it on mainnet as well :)

👍 ajtowns approved a pull request: "test: cover invalid codesep positions for signature in taproot"
(https://github.com/bitcoin/bitcoin/pull/32301#pullrequestreview-3413032094)
ACK 81e5c8385b9ec170c97190a97c560a39ccfc544a

💬 roconnor-blockstream commented on pull request "Fix BIP143 standardness rules for CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33759#issuecomment-3482731935)
I don't think the wording "passed to a sigop" is ambiguous at all, but I'm fine with the BIP text being clarified if someone wants to do that instead.

I don't have a strong opinion on how to resolve the disagreement. I do think that updating the implementation is slightly better, but only slightly.