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

💬 luke-jr commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3483326655)
**Regarding DNS seed policies**, my seed is fully compliant.

It has always intentionally lagged introducing new versions, in case of unexpected issues, and this has never been brought up as a potential "breach" previously (indeed, other seeds also select for specific versions). Nor does it in any way contradict any explicit or implied purpose of DNS seeding. Part of the intent in this lagging is to increase diversity of results compared to other DNS seeds.

For the record, the current range of
...

💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3483434223)
> 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:

I see two misses for this approach (though I'm still not looking at any utxos after block 400k):

* [7be22151bc96041932830d79c2af4472e40e9dedaa29018fff6757501e7855c6:0](https://mempool.space/tx/7be22151bc96041932830d79c2af4472e40e9dedaa29018fff6757501e7855c6) 20000 sats, block 344398 (two 65 byte data pushes on top of a compressed ke
...