💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2060028715)
Fixed typo since I had to retouch anyway.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2060038062)
I replaced the four `state.GetRejectReason()` with `state.ToString()`, as it was in the original.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2060045258)
Making this return a `BlockValidationState` means we have to pass it over the interface, which means we can't drop the special handling, see cca5993b26e6223af31fe1ef5bf8a319cb87cf93.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059714722)
In "txgraph: Add GetMainStagingDiagrams function (feature)" e1cb50a9574b70ae6509fc3578af3cbf886f2b63
nit
```suggestion
auto sim_gain = sims[1].SumAll() - sims[0].SumAll();
// Just check that the total fee gained/lost and size gained/lost according to the
// diagram matches the difference in these values in the simulated graph. A more
// complete check of the GetMainStagingDiagrams result is performed at the end.

...

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059723445)
In "txgraph: Add GetMainStagingDiagrams function (feature)" e1cb50a9574b70ae6509fc3578af3cbf886f2b63

The sorting in `GetMainStagingDiagrams` uses the > operator which additionally compare the size when their is tie, whereas here we use `FeeRateCompare` which only compare the fee rates.

Shouldn't this and others below also use the > operator?

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2793240075)
Code review ACK a0becaaa9c7390bc80d5864f7fffb32e21502a50

Comments are just nits and some questions.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059731062)
In "txgraph: Add GetMainStagingDiagrams function (feature)" e1cb50a9574b70ae6509fc3578af3cbf886f2b63


They both have to not be oversized no?

```suggestion
if (sims.size() >= 2 && !sims[1].IsOversized() && !sims[0].IsOversized()) {

```

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059726730)
In "txgraph: Add GetMainStagingDiagrams function (feature)" e1cb50a9574b70ae6509fc3578af3cbf886f2b63

Shouldn't this be main_sim_diagram?

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059744992)
In "txgraph: Add GetMainStagingDiagrams function (feature)" https://github.com/bitcoin/bitcoin/commit/e1cb50a9574b70ae6509fc3578af3cbf886f2b63

Is this not redundant since they sorted already?

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059777287)
In "txgraph: Add GetMainStagingDiagrams function (feature)" https://github.com/bitcoin/bitcoin/commit/e1cb50a9574b70ae6509fc3578af3cbf886f2b63

We say the graph must not be oversized; but do nothing when it is and still compute the fee rate diagrams.

It will return the fee rate diagram but the chunks might not be in acceptable state.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2059755613)
In "txgraph: Add GetMainStagingDiagrams function (feature)" https://github.com/bitcoin/bitcoin/commit/e1cb50a9574b70ae6509fc3578af3cbf886f2b63

nit: here and another comment below.
```suggestion
// here, but it has to be consistent with the one used in main_real_diagram and
// stage_real_diagram).
```

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2830171878)
Sorry for a few useless pushes, I was fighting some compilers for strings not always being `constexpr`-able...
Code is ready for review again.

💬 laanwj commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2830172599)
> This should probably be going upstream (https://github.com/arun11299/cpp-subprocess). cc @hebasto.

Same for the subprocess commit of #32343. Optimizing close-all-file-descriptors is out of scope here but it's apparently needed to even pass the CI.

💬 pablomartin4btc commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2830196056)
I've tested https://github.com/bitcoin-core/gui/commit/71656bdfaa6bfe08ce9651246a3ef606f923351b on Ubuntu 22.04, trying several times as well with the steps mentioned in #862 and I can't reproduce the bug anymore.

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2830252505)
TIL Linux has `close_range(first, last, flags)` which would be ideal for this, as it closes an entire range of file descriptors in one syscall. But unfortunately it was introduced in glibc 2.34, and we require only 2.31.

🚀 fanquake merged a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2060128901)
I changed `TestBlockValidity` to also take a `debug` argument.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2830264751)
Rebased, fixed typo found by LLM, clarified non behavior change for `proposal` mode.

I added a `debug` argument to pass along more detailed failure reasons.

I'm a bit on the fence regarding this suggestion: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2035713597

I also still need to look at the sigops check.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059839540)
nit in 20a9173717b1aa0d0706894f8bda47492e1d71a9: Not that it matters, but shouldn't this commit set `self.add_wallet_options(parser, legacy=False)`?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059892742)
in c847dee1488a294c9a9632a00ba1134b21e41947: I don't think this comment is correct. It is true that `wallet_names` are ignored in `setup_nodes`, but it is still possible to manually call `import_deterministic_coinbase_privkeys` or `init_wallet`.

My recommendation would be to remove the comment, or replace `we` with `setup_nodes`.