🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2718512065)
reACK b2ea3656481b4196acaf6a1b5f3949a9ba4cf48f 🚀

💬 fjahr commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2755649223)
@jurraca I think you should remove the tag of sipa in the description (just change to without the @), otherwise github is a bit spammy because the description is included in the merge commit. (Or maybe that is fixed now but that's something that used to happen)

💬 TheCharlatan commented on pull request "test: Add functional test for bitcoin-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014940033)
The tests by design only capture the very basic functionality. Confirming the output of the rest seems a bit overkill.

💬 darosior commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2755664374)
I've tested to run a downstream project's ([Liana](https://github.com/wizardsardine/liana)'s) functional test suite against v29.0rc2. It did not surface any bug nor RPC interface regression. I also ran it with a `bitcoind` under Valgrind.

I've also been running a (non-listening) v29.0rc2 mainnet node under Valgrind for a few days, sanity checking various operations but nothing really involved.

I've also tried to get end users to test the new port mapping implementation on whatever router they
...

💬 TheCharlatan commented on pull request "test: Add functional test for bitcoin-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014949136)
I used `assert_equal` first, but then saw that partial matches use a similar approach here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/test_node.py#L523.

💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#issuecomment-2755672096)
I have measured these commits separately against a baseline (undoing each commit, to measure them independently).
* 1d281daf86 `Merge bitcoin/bitcoin#32095: doc: clarify that testnet min-difficulty` is the baseline
* 1112433318 `SaltedOutpointHasher uses rapidhash` is 5% faster than 1d281daf86
* e00d828362 `CCoinsViewCache::BatchWrite lookup optimization` is 0.5% faster than 1d281daf86
* 1630e368d1 `Use boost::unordered_node_map for CCoinsMap` is 7.4% faster than 1d281daf86

<details>
<su
...

💬 santochibtc commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2755724330)
@darosior No, There is nothing about natpmp in my log. UPnP was enabled by default

🤔 glozow reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2714830754)
ACK b2ea3656481

Reviewed the structure to convince myself this approach makes sense to support the features required, and reviewed the `SimTxGraph` + differential fuzzer in closer detail. I haven't read through all the `TxGraphImpl` functions but did some manual mutation testing.

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2012730654)
nit c80aecc24ddd878c62be9753a2746e36860e3a97:

now that this variable has type `GroupEntry` instead of `GroupData`, it would be more clear to name it something like `group` or `group_entry`

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2012786771)
nit in 8c70688965bc4038f28f41e4490180e40a88b5ee

It could be worth assigning constants `MAIN_LEVEL{0}` and `STAGING_LEVEL{1}` to replace the 0s and 1s in the file?

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2012836894)
Some parts of the file seem to suggest a max_level-agnostic implementation (e.g. looping on `level` from 0 to `GetTopLevel()`), but this casting from a bool to an int suggests it has to be 2. I think it would be useful to have more levels (e.g. for package test acceptance) in the future, but perhaps a `static_assert(MAX_LEVELS == 2)` might be good documentation here?

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2014960767)
fwiw, I don't think there is test coverage for this (i.e. fine if this is always set to optimal), though I expect any problems would become more obvious when mempool things are hooked up to txgraph

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2015001561)
Why can't `FindConnectedComponent` be used instead?

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2015003889)
nit 0aa874a357865dd4768091f26dff238e66fb8d83
```suggestion
// elem needs to be placed before anymore, and place_before would be empty.
```

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2015005058)
in 05abf336f997f477c6f48412809ab540fccf1cb0

I suppose this stops us from making removed larger than MAX_TRANSACTIONS + 100?

💬 glozow commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2015002217)
Is this necessary given that the fuzzer already did `pick_fn` randomly?

💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r2015015995)
Yepp, I actually ran into this during my first iteration of working on this. I will add a comment to explain it.

💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-2755777136)
I finally got back to this today and it drove me nuts many different ways! The primary source of frustration was a my lack of experience with fuzzing tests + recent changes on the build system and where the bins are placed + this PR not having these changes but me thinking it had these changes + fuzzing and macos dislike each other. Thanks to @marcofleon for support and hence the rebase here!

However, I did eventually also find something interesting! :) @marcofleon had already mentioned to me
...

🚀 glozow merged a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363)

💬 jimhashhq commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-2756056144)
As promised in #10102 comments, I tried out `-ipcconnect` using rebased pr-19641 (thank you); everything seemed to work consistent with my new understanding of multiprocess interactions (thanks again). I did notice one issue:

1) If `bitcoin-node` is run with -`disablewallet`. A subsequent start of a gui with`-ipcconnect=auto`, but _without_ `-disablewallet` will crash `bitcoin-node`. This is obviously an operator error, but may be worth handling w/o node crash. Error is:

> 23329 Segmenta
...