💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2829997330)
Just letting the thread know that we've cleared up our miscommunication regarding https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2788618744. The combination of my prior suggestion failing CI and @l0rinc's cautioning to make sure my further suggestions pass CI seem to be an unlucky coincidence within a very short time-frame. We should strive towards assuming good-faith when communicating over text, but on some days frustrations can get the best of me.

💬 fanquake commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2830017198)
https://github.com/bitcoin/bitcoin/actions/runs/14651550530/job/41119318297?pr=32345#step:5:966:
```bash
Assertion failed: fRPCInWarmup, file ./rpc/server.cpp, line 329
Error: Process completed with exit code 3.
```

https://cirrus-ci.com/task/5477140512112640?logs=ci#L5037:
```bash
[17:33:37.941] ./test/node_init_tests.cpp(41): error: in "node_init_tests/init_test": check AppInitMain(m_node) has failed
[17:33:37.941] Error: Cannot resolve -bind address: ''
[17:33:37.954] ./test/node_i
...

💬 maflcko commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2830043656)
The no wallet CI task passes: https://cirrus-ci.com/task/5884997988515840?logs=ci#L3274

What is the configure summary?

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