💬 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

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3482794144)
> I'm not sure whether to Concept ACK/NACK this...

Since this PR is only a subtask of the broader effort to resolve https://github.com/bitcoin/bitcoin/issues/30210, it seems more appropriate to have a conceptual UCRT-related discussion there on in https://github.com/bitcoin/bitcoin/pull/33593.

This one simply addresses your comments [here](https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3460836590) and [here](https://github.com/bitcoin/bitcoin/pull/33593#pullrequestreview-339869
...

💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3482836515)
At the risk of opening the bikeshed, another reasonable option I was thinking of today is allowing arbitrary data up to 33 bytes, or an uncompressed pubkey:

bool static IsDataOrUncompressedPubKey(const valtype &vchPubKey) {
if (!vchPubKey.empty() && vchPubKey.size() <= CPubKey::COMPRESSED_SIZE) {
// A "data" push or a compressed pubkey.
return true;
}
if (vchPubKey[0] == 0x04 && vchPubKey.size() != CPubKey::SIZE) {
// An un
...

🤔 stickies-v reviewed a pull request: "prevector: simplify implementation of comparison operators and match behavior of `std::vector`"
(https://github.com/bitcoin/bitcoin/pull/33772#pullrequestreview-3413315690)
I like the idea, and it would be an improvement to both simplify the prevector implementation and move it closer to being a `std::vector` drop-in. However, the different `operator<` causes quite widespread behaviour change (not just direct comparison such as in `CNoDestination::operator<`, but also in e.g. all maps and sets of `CScript`), and assuring myself the changes are safe is going to take more time than seems worth it for the stated benefits. So: Concept ACK, but Priority NACK for me.

💬 luke-jr commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3483040703)
Concept NACK, they "soft confiscated" their own coins. This isn't a good excuse to enable spam.