💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535342513)
In commit "fuzz: remove comparison between mini_miner block construction and miner"

Ok. This commit belongs much earlier in the PR, though (just before "Select transactions for blocks based on chunk feerate"). Otherwise, the range of commits in between will fail fuzz tests if run long enough.

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535291747)
In commit "squashme: only output weight, not vsize, for cluster/chunk information"

Squash the squashme?

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535278410)
In commit "Rework RBF and TRUC validation"

The `GetParents` function is only introduced in the next commit.

📝 joshdoman opened a pull request: "[kernel] Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891)
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.

Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.

I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bit
...

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535364299)
> Not sure why we'd want to reduce dependencies here, what's the advantage of that?

I wrote it above; the idea is to be able to use this low-level class and tests elsewhere, outside the project, just by pulling a few files without dragging in all our unit test framework machinery, which has tons other dependencies.
If we were talking about a substantial improvement, I’d agree with you, but here it’s just a 2-line diff with no behavior change. And for me, that makes the rationale for includin
...

💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3543642952)
@ryanofsky should we close this?

💬 achow101 commented on pull request "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members":
(https://github.com/bitcoin/bitcoin/pull/32763#discussion_r2535396633)
Done

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2535398529)
had a closer look now, it actually is straightforward using `GetDebugMessage()`. So I added the log.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3543689545)
[6d2c8ea](https://github.com/bitcoin/bitcoin/commit/6d2c8ea9dbd77c71051935b5ab59224487509559) to [6db2551](https://github.com/bitcoin/bitcoin/commit/6db2551dc27c4a9b989e8814054c93dd9d8f1b36):
Added hash of invalid block to the log.

📝 instagibbs opened a pull request: "policy: Remove individual transaction <minrelay restriction"
(https://github.com/bitcoin/bitcoin/pull/33892)
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.

In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the a
...

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535330614)
Good point regarding .clang-format, though that value from 13f36c020f0329b5e975282b45292fdf2a495e31 comes from clang-format defaults rather than a conscious decision on our part. (It will not remove newlines at EOF if they exist though, https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof).

GitHub displays a red warning symbol in diffs when one removes the empty newline at ends of files.

`git apply diff.patch` fails for...
```diff
diff --git a
...

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535372567)
Thanks! (I believe that's fixed in this PR - 093980af8e4e594716b9219931fd441266c1fcd4 "init, net: Implement usage of binary-embedded asmap data")

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535368296)
Ah, wasn't on my radar, cheers! Does simplify things a bit.

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3543708460)
https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432472005 still has invalidateblock in `test/functional/mempool_packages.py` ? If you don't have the time to carry the patch I can take over, just let me know.

🤔 hebasto reviewed a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869#pullrequestreview-3474450076)
post-merge ACK.

🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3455337470)
Code reviewed the "squashme"s (I think they should be squashed) and the end state for MemPoolAccept, miner, reorg, src/policy/* changes. I plan to put most of my comments on #33591. My suggestions here are to clean up the commits so the git history is easy to trace in the future.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519820632)
ccf7c5ac417 looks good and can be squashed

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519700544)
nit if you retouch: commit message for cec26203732 should replace "descendantsize" with "descendant count"

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519770412)
If you retouch: f02ff837008 can be squashed into the commit above it. I think the diff may have shrunk over time - the commit message mentions `addUnchecked` which no longer exists in the codebase.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519738828)
nit in 1eddff0b0ab if you retouch: `GetDescendantCount`, `GetAncestorCount` would be more similar to existing naming, and slightly less confusing since the count includes the tx itself.