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

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-3138094678)
> I would suggest a followup PR to improve various code comments and such.

I can pull them into #29675

💬 achow101 commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2244047141)
I've changed this to use the existing `ser_string` utility which is more accurate.

💬 achow101 commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2244047413)
Done as suggested.

🤔 jlest01 reviewed a pull request: "test: Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060#pullrequestreview-3073614732)
ACK https://github.com/bitcoin/bitcoin/pull/33060/commits/a3cf623364e84819bc16fd407b80d8dba46bbcb5

It improves the BnB coin selection tests by covering previously untested weight-limit edge cases and verifying the proper error flagging.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2244054257)
> it only ratchets hardened from false to true

No? It resets it to false first.