💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2059997721)
> nit: CheckSignatureEncoding already checks for empty sig, so the first check isn't needed

I checked that `CheckSignatureEncoding` returns `true` for an empty signature.
https://github.com/bitcoin/bitcoin/blob/ff69046e664f2460bb2ef595c5bd16eeb4a13f58/src/script/interpreter.cpp#L200-L205

But here the requirement is to throw error in that case because of what's stated in [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki).

> The signature as would be pushed to the
...

💬 fanquake commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2830057461)
```bash
Configure summary
=================
Executables:
bitcoind ............................ ON
bitcoin-node (multiprocess) ......... OFF
bitcoin-qt (GUI) .................... ON
bitcoin-gui (GUI, multiprocess) ..... OFF
bitcoin-cli ......................... ON
bitcoin-tx .......................... ON
bitcoin-util ........................ ON
bitcoin-wallet ...................... OFF
bitcoin-chainstate (experimental) ... OFF
libbitcoinkernel (experimental) ..... OFF
Optional
...

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2060028571)
Good point. The original code would return a string in some cases and throw an error in others. Although it's possible nobody uses the `getblocktemplate` RPC in `proposal` mode at all, there could be (unmaintained) proprietary software out there using it. So the safest option is to maintain the behavior.

However it seems there's no change in behavior: `BIP22ValidationResult` only throws for `state.IsError()`, not for `state.IsInvalid()`. I looked through `CheckBlock` and the functions it call
...

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