👍 theuni approved a pull request: "cmake: Proactively avoid use of `SECP256K1_DISABLE_SHARED`"
(https://github.com/bitcoin/bitcoin/pull/33101#pullrequestreview-3072902914)
Nice. I've been wanting to scope the secp `add_subdirectory` for a while now.

utACK d5d95035bc6ef135476d7f734c574b38d6faaf70. Reviewed with `git diff --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change`

💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3137441579)
> Thank you, fixed

And fixed the one a few lines after too (thanks @marcofleon)

💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3137446026)
Yes, we could do that today. I suppose the minimal way to do so today on master would be to simply check whether we are spending any Segwit input while the transaction has no witness after `CheckInputScripts` failed:
<details>
<summary>Expand patch</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 09e04ff0ddb..8f86b630ef5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1254,13 +1254,17 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& arg
...

💬 glozow commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3137458271)
> However it seemed cleaner to me to get rid of the WITNESS_STRIPPED edge case detection in the first place, which was always meant to go?

I didn't realize it was always meant to go - how do you know that?

Fwiw, I do think that being able to detect `WITNESS_STRIPPED` without triple validation is the main thing we want, so would prioritize that kind of solution. At the same time, my main point in the OP is that being able to cache `INPUTS_NOT_STANDARD` isn't very helpful to us in practice,
...

💬 theuni commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3137474121)
> (On the other side, I'm also not sure about the benefits of this PR. PR description links to a lot of issues and might be clearer if it just described the problem it solves, or explained if the main movitation is simplifying cmake code or reducing the number of installed files or something else.)

At a high level, this simply encapsulates the kernel library. The shared lib already works this way, this PR would just align the static lib's behavior. The other dependencies (crypto, leveldb, se
...

💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3137486313)
> I didn't realize it was always meant to go - how do you know that?

It was introduced in https://github.com/bitcoin/bitcoin/pull/18044 as a hack until the network upgrades to wtxid relay with an explicit mention it can be (according to author, [should be](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r388445400)) removed afterward:
https://github.com/bitcoin/bitcoin/blob/8a94cf8efebc3177effcfc1160560735b8caf34b/src/node/txdownloadman_impl.cpp#L452-L454

Of course that it was *m
...

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3137510651)
Rebased e4d80d721b8336047fcd75eb1891e08ac156a903 -> 81387424d37650e186535b9868d71c5882bc7b20 ([kernelInternalLib_18](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_18) -> [kernelInternalLib_19](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_19), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_18..kernelInternalLib_19))

* Fixed conflict with #31829

🚀 hebasto merged a pull request: "Move `FreespaceChecker` class into its own module"
(https://github.com/bitcoin-core/gui/pull/881)

💬 mzumsande commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3137595081)
> Hi furszy, I thought the test I put already demonstrate the issue, though it is not proper to be included to the codebase.

I agree that a test that copies / amends the actual productionc code is not great for inclusion, but why not include the test https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38 from above that doesn't have this problem?

💬 mzumsande commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3137646098)
ACK 4cb2c0520211634ee3caf6d4c3a1e65b2bdb2dc6

My comment above is not blocking (could also be done later) - considering that this is tagged for 30.0 and feature freeze is soon, I wonder if we should aim for that, or combine it with #26966 (which I think could be enabled for coinstatsindex relatively easily, FYI @furszy) so that all of the users with existing indexes would have the option to resync much faster.

📝 darosior opened a pull request: "validation: detect witness stripping early on"
(https://github.com/bitcoin/bitcoin/pull/33105)
Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input.

Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package
...

💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3137704209)
I opened https://github.com/bitcoin/bitcoin/pull/33105, which ended up implementing a variant of @ajtowns' suggestion, because as pointed out the resource consumption gains can be achieved independently of this work (which i still think is desirable) as a much smaller patch which i think would be nice to get in before the feature freeze.

💬 achow101 commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-3137786763)
ACK 9954d6c833381a44e1ea34d182ffe4d61b65d2ba

🚀 achow101 merged a pull request: "init: [gui] Avoid UB/crash in InitAndLoadChainstate"
(https://github.com/bitcoin/bitcoin/pull/32987)

🚀 achow101 merged a pull request: "depends: hard-code necessary c(xx)flags rather than setting them per-host"
(https://github.com/bitcoin/bitcoin/pull/32584)

🤔 jlest01 reviewed a pull request: "refactor: remove unused `ser_writedata16be` and `ser_readdata16be`"
(https://github.com/bitcoin/bitcoin/pull/33093#pullrequestreview-3073357661)
ACK https://github.com/bitcoin/bitcoin/pull/33093/commits/0431a690c3a498a1e728c9df34a132ac16177a04

💬 gmaxwell commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3137886715)
I don't see why all standardness rules can't be validated prior to doing any costly hashing or signature validation (sadly, they do require looking up inputs which is costly).

If indeed standardness validation happens before costly operations then the fact that they drop txn without disconnecting the peer doesn't really tell us anything about what should be done *after* performing expensive operations.

But most critically the primary reason to disconnect (and potentially keep disconnected)
...

💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2243912114)
I also think it would be better to drop the `m_block_*` class members since they're basically only used by `CustomAppend`.

💬 achow101 commented on pull request "test: Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3137945940)
ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5

💬 achow101 commented on pull request "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys":
(https://github.com/bitcoin/bitcoin/pull/32332#issuecomment-3138020102)
-0

I don't think such refactorings are meaningfully useful. While it may improve readability a bit, I don't think the difference is enough to justify making this change. Such refactorings both can cause merge conflicts with other higher priority work, and also make git blame a little bit harder.