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

🤔 pablomartin4btc reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3073861902)
Concept ACK

This refactor replaces explicit `.count(x)` checks with `.contains(x)`, which only verifies that at least one instance exists (or none when `== 0`). Please double-check whether `.contains()` preserves the original behavior in some contexts where exactness matters ( !=1, ==1, etc). Also where the logic enforces that exactly one item is/ is not present — useful even in containers that don't allow duplicates (like `std::map` or `std::set`) because it clearly expresses the expected lo
...

🤔 l0rinc reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3073908954)
Thanks, split up the PR into 3 commits to simplify assessing the risks - hope this clarifies it, see the commit messages for further details.

💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#discussion_r2244294569)
I had these at one point, but decided against modifying multi-containers - but this one seems fine, thanks, added.