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

👋 l0rinc's pull request is ready for review: "[IBD] flush UTXO set in batches proportional to `dbcache` size"
(https://github.com/bitcoin/bitcoin/pull/31645)

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-3138505865)
Rebased, the PR is ready for review again!

The batch size for UTXO set writes is now calculated based on the maximum `dbcache` size to ensure that with the default values, memory usage doesn't increase, while reducing flushing time when there is enough memory available.
The change reduces the IBD time by a fixed amount, it's speeding up a critical part of saving the state for long-term storage.

💬 nervana21 commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3138622075)
tACK [c157438](https://github.com/bitcoin/bitcoin/pull/33083/commits/c1574381168573c561ebddf1945d2debefb340f7)

I reproduced the errors as expected with the provided diffs. I validated that the master branch tests pass when the proposed commits are applied.

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3138628456)
> > 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 production code is not great for inclusion, but why not include the test [furszy@f2b8a06](https://github.com/furszy/bitcoin-core/commit/f2b8a06060e2a313f35101454c7669d46e0edc38) from above that doesn't have this problem?

I see, sorry for somehow I missed that part. I thought the test furszy mentioned is my tes
...

💬 nervana21 commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#discussion_r2241408531)
Small nit

```suggestion
# The failure above was cached. Submitting the compact block again will return a cached
```

✅ rkrux closed a pull request: "Musig2 fields followups"
(https://github.com/bitcoin/bitcoin/pull/32412)

💬 rkrux commented on pull request "Musig2 fields followups":
(https://github.com/bitcoin/bitcoin/pull/32412#issuecomment-3138649267)
Closing this one now.

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3138661911)
> @HowHsu, are you going to include the test or a patch to reproduce the issue? I’m happy to ACK it either way.

Hi @furszy , sorry for the delay, I've picked your commit of the test, which I missed earlier.