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

💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641)
Concept ACK 69c015b258c3bbfb44af478d7a5a05ad0b0156b8 and I've started reviewing this. Overall I think this is a nice change that simplifies net.cpp by removing low level socket calls and details of establishing connections from implementation of the transport protocol. It also seems like it should make net.cpp code more portable if for example we wanted it to use libuv, or a future c++ networking library, or experiment with sandboxing, mesh networking, etc. The [sockman API](https://github.com/v
...

💬 yuvicc commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3483658143)
Ready for review.

💬 delta1 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-3483710103)
@luke-jr since you consider Core v30 to be “malware”, does that mean your seeder will never support returning v30 nodes?

🤔 yuvicc reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3302473141)
Approach ACK

Did a round of review while updating the [java-bitcoinkernel](https://github.com/yuvicc/java-bitcoinkernel/pull/4) bindings and testing the API.

Left some nits, mostly documentation consistency and minor code improvements.

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2404587136)
Can we mark the context as const here?

```suggestion
const btck_Context* context) BITCOINKERNEL_ARG_NONNULL(1);
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488609371)
```suggestion
* `btck_ScriptVerificationFlags_WITNESS` flag is set in the flags bitfield, the
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488614705)
```suggestion
* @param[in] flags Bitfield of btck_ScriptVerificationFlags controlling validation constraints.
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488618692)
```suggestion
* @param[in] category If btck_LogCategory_ALL is chosen, all categories will be enabled.
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488601949)
```suggestion
* read block and undo data from disk, and validate scripts. The header is
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2404583482)
I think we can use `int32_t` here like we've used in `btck_block_tree_entry_get_height` method?

```suggestion
BITCOINKERNEL_API int32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_chain_get_height(
```

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488671851)
Could be more specific
```suggestion
* @param[in] output_index The index of the transaction output to be retrieved.
```