🤔 sdaftuar reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2167121806)
Code review ACK cad318fa843f411e52c6761a4882bfaf0ad21812. Left some non-blocking nits/suggestions.

I've run the fuzz tests in earlier versions of this PR for a while, but I haven't done so with the latest branch -- will do that and also post some benchmarks in a bit.

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1671201334)
Perhaps worth documenting that if an existing linearization is passed in, it must be topologically valid, or else the output of `Linearize` may not be topologically valid.

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1671088434)
nit: s/the the/the/

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1676094213)
Given that there's more than one valid serialization that will deserialize to the same graph, perhaps it's worth commenting something to that effect here? (I guess we could also consider dropping this check and instead just checking that if we deserialize `hexenc` we get the same depgraph, but perhaps it's beneficial to check that what we think of as a canonical encoding is correct?)

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690139736)
Maybe a different variable here would be better, since `i` is already used in the outer loop?

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688552973)
We could consider checking that the entries in `linearization` are all in the expected range [0, depGraph.TxCount()), to avoid a crash here if a garbage linearization were passed in?

👍 hodlinator approved a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2196468742)
ACK fac0c3d4bfc97b94f0594f7606650921feef2c8a

`make check` passed and `test_runner.py` passed.

Happy you went with `FromHex()`!

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689620422)
Man, I was worried for a moment that this modification of switching to the `uint8_t` was setting the other end of the `base_blob` array, but it's the same. Will be glad to see the fragile function go. :+1:

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2248546746)
Benchmark results on my ryzen 7995wx:

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,474.01 | 678,422.17 | 0.3% | 0.01 | `LinearizeNoIters16TxWorstCaseAnc`
| 1,473.48 | 678,667.50 | 0.2% | 0.01 | `LinearizeNoIters16TxWorstCaseLIMO`
| 4,763.02 | 209,951.04 | 0.1% | 0.01 | `LinearizeNoIters32TxWo
...

💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2248589227)
re: https://github.com/bitcoin/bitcoin/pull/30509#issue-2425377062

> The default socket path is currently `<datadir>/sockets/bitcoin-node.sock`, but this can be customized.

I changed the default socket path to `<datadir>/node.sock` after running into some "exceeded maximum socket path length" errors. The maximum unix socket path length is only 107 characters, so I thought a shorter suffix might avoid problems on systems with long data directory or home directory paths.

Another possible
...

👍 willcl-ark approved a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2197582258)
ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988

Looks good to me with those nits fixed 👍🏼

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690313716)
Attempted cleaning that up, but didn't uncover anything useful to the end users, unlike #30444, so probably not worth following up on:

```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 89c03c1647..dab8b81128 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -274,8 +274,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu

// extract and validate vout
const std::string& strVout = vStrInputParts[1];
- int64_t vou
...

💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690338607)
nit: After this, the log
`Committed X changed transaction outputs (out of Y) to coin database...`
has become less interesting because we only iterate over dirty coins, so `X=Y` always.
I might be more helpful to report the initial size of `m_map` of the cursor (before entries are deleted from it) as the total.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690361434)
There are other places that could be cleaned up, such as obviated dirty checks like [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-cafbe1353eff6084b73fd3b6c3dee603e0827348fdd2fe12dfad1e01003a84edR118) and [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR195), but this should probably done after a https://github.com/bitcoin/bitcoin/pull/18746 revival so there's no confusion.

💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690365906)
yes, no need to do that here. I just mentioned the logging here because I was interested in the ratios to assess the efficiency of this PR, and had to write my own log.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690367507)
Also, that line predates #17487, back when all entries were dirty anyways.

👍 TheCharlatan approved a pull request: "guix: GCC 12 consolidation"
(https://github.com/bitcoin/bitcoin/pull/30511#pullrequestreview-2197680394)
ACK d1592d2eee1913e734a4f92907e796eb3350c64a

Guix build (aarch64):
```
4fba0aba8be59fd967b099e19faf6ba6770a83de1e7bee745c16f91fa3eb92fe guix-build-d1592d2eee19/output/aarch64-linux-gnu/SHA256SUMS.part
37e93c9235db88f58e5a45e8a54e1d57602f9107cd23676c91252509b2fe3bb5 guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu-debug.tar.gz
184fb61be0843b069c4c0d92119b9e29e895f11ef8687b220ab3898999c06afa guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d
...

💬 TheCharlatan commented on pull request "guix: GCC 12 consolidation":
(https://github.com/bitcoin/bitcoin/pull/30511#discussion_r1690370855)
Hope this gets merged soon so we can drop this again.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385057)
Done.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385537)
Added the mermaid code as comments to the benchmark.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385632)
Fixed.