💬 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())) {
```