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

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487946786)
I see you've moved it as well, clang complains with:
> Clangd: Moving a temporary object prevents copy elision

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487950675)
nit: consider formatting it as `10'000` for readability

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487956680)
interesting, it does work for me, but inline might indeed be problematic in this case.
Removing it should be fine, please resolve this comment.

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3482770283)
The feedback has been addressed.

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2487976102)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3482770283).

💬 TheCharlatan commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2487982976)
Mmh, I think that could clarify a bit how the sigops are accounted for, but if you can read the test, you can also just go and read the original code? I guess it could still guard against dumb regressions of the accounting code though.

💬 TheCharlatan commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2487975369)
This comment is misleading. It is not just the witness that is not taken into account, but also any p2sh sigops.

💬 TheCharlatan commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2487977414)
Can you add a case to the multisig case too? There we should be expecting a sigop cost, even if the first input's prevout is null.

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