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

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3138129596)
Added a few commits to address followups from #31244

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3138169095)
Rebased and added a new commit on top for deduplicating the integer rounding. Ready for review again.

👍 brunoerg approved a pull request: "test: Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060#pullrequestreview-3073687261)
code review ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5

💬 ajtowns commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3138260890)
> 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 don't think it makes sense to get rid of WITNESS_STRIPPED while we still care about resolving orphans via their missing parents' txids. If/when we have protocol support for receiver-initiated package relay by wtxid, then I think it could make sense to treat every request as being by wtxid -- so a witness-stripped response from a non-wtxid-relay peer woul
...

💬 luke-jr commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3138265471)
Concept NACK. Everything below 1s/vB is spam. There's no reason to change the default.

>Over that period the USD price of BTC has risen by roughly 2-3 orders of magnitude,

It's the USD that has fallen, Bitcoin has remained more or less the same.

>This maintains DoS resistance without discouraging low-value UTXO consolidation.

DoS resistance is clearly insufficient even at 1s/vB.

💬 luke-jr commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3138268696)
Actually, Bitcoin probably hasn't even kept up its value, so if anything we should be looking to *increase* the default relay fee, if maintaining the same actual-value cost is the goal.

🤔 ajtowns reviewed a pull request: "validation: detect witness stripping early on"
(https://github.com/bitcoin/bitcoin/pull/33105#pullrequestreview-3073725534)
Approach ACK

💬 ajtowns commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2244144979)
Maybe put this logic in policy.h and call `if (RequiresNonEmptyWitness(prev_spk))` or `if (RequiresNonEmptyWitness(tx))`?

💬 ajtowns commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2244141585)
Shouldn't this check also be gated by `m_opts.require_standard` ? I guess that could be done in #29843 presuming this is merged first.

💬 OreoCookieRustDev commented on issue "Pruned node missing some coinbase UTXOs (version: 28.1)":
(https://github.com/bitcoin/bitcoin/issues/33071#issuecomment-3138312655)
I marked it closed cause I was stupid. I did mean block 8 but looking up by address doesn't work for coinbase one has to look up by pk

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2244205957)
> The true p2p-specific code in the mempool appears to be the existence of sequence numbers, but getting rid of that seems like a much bigger code change than is warranted for this follow-up PR.

I have a bunch of thoughts about separating "kernel" and "relay" mempool logic like this... hmm, maybe should just do a brain dump to a gist sometime.

> Non-mempool code using `mapTx` iterators imo remains an anti-pattern

I think if they were (treated as) opaque handles and you had to use `GetIn
...

💬 pablomartin4btc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#discussion_r2244246962)
nit: not this one?
```suggestion
if (mapModifiedTx.contains(it) || inBlock.contains(it->GetSharedTx()->GetHash()) || failedTx.contains(it->GetSharedTx()->GetHash())) {
```