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

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059973381)
This test doesn't seem wallet related? Why is this set?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059965247)
why is the call removed? Either the function should be removed along with the call, or the call should be kept

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059961431)
forgot to remove `enable_wallet_if_possible`?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2060029160)
forgot to remove this?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2059977759)
unrelated in c847dee1488a294c9a9632a00ba1134b21e41947: However, while touching, it could make sense to force named args for literal args, especially integral (boolean) ones:

```py
def __init__(self, rpc, *, cli=False):

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2060139068)
Also, you forgot to remove `REQUIRE_WALLET_TYPE_SET` in this commit.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2060010792)
why remove bdbro and swap-bdb-endian? The options remain, so either they should be tested, or fully removed.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2060088046)
forgot to remove LEGACY_WALLET_MSG?

💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2059993955)
This change assumes their tip is higher than our tip. What if their tip is lower? Maybe worth advancing `pindexLastCommonBlock` in that case as well. Something like this:

```cpp
// If our tip has advanced beyond pindexLastCommonBlock, move it ahead to the tip. We don't need to
// download any blocks in between, and skipping ahead here allows us to determine nWindowEnd better.
const auto our_tip{m_chainman.ActiveTip()};
const auto peer_best_known{state->pindexBestKnownBlock};
if (our_tip-
...