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

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2244450698)
Ah yes, in that case:

```
@param[out] has_hardened whether hardened derivation was found
```

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2244453674)
The version in https://github.com/bitcoin/bitcoin/pull/29675/commits/24bcdd27fd6c32646eb87eb8bea6f539bbc54e42 is fine.

💬 maflcko commented on pull request "refactor: remove unused `ser_writedata16be` and `ser_readdata16be`":
(https://github.com/bitcoin/bitcoin/pull/33093#issuecomment-3138711573)
lgtm ACK 0431a690c3a498a1e728c9df34a132ac16177a04

💬 luke-jr commented on pull request "doc: move `cmake -B build -LH` up in Unix build docs":
(https://github.com/bitcoin/bitcoin/pull/33088#issuecomment-3138752685)
(might make sense to mention `ccmake` too?)

💬 instagibbs 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-3138794818)
crACK c1574381168573c561ebddf1945d2debefb340f7

💬 instagibbs commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3138821243)
ACK ea17a9423fb431a86d36927b02d3624f654fd867

I think the doc update gives a decent explanation how this can be used. Code cleanup is nice.

💬 instagibbs commented on issue "doc: Mempool Policy documentation Outdated since TRUC":
(https://github.com/bitcoin/bitcoin/issues/32067#issuecomment-3138855557)
You are correct that this no longer holds with TRUC. The caveat can be added.