👍 fanquake approved a pull request: "guix: Remove unused `file` package"
(https://github.com/bitcoin/bitcoin/pull/32242#pullrequestreview-2793602829)
ACK 513e2020a9acdd366d4933780b331f97bac85597

🚀 fanquake merged a pull request: "guix: Remove unused `file` package"
(https://github.com/bitcoin/bitcoin/pull/32242)

💬 maflcko commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2829957978)
I guess it could make sense to append `-O2` in the case of `-DCMAKE_BUILD_TYPE=RelWithDebInfo` reported in OP as well, to avoid the unexpected `-O3` there? (The output reading a bit odd seems like a style issue that can be ignored for now)

⚠️ fanquake opened an issue: "test: functional test failures under -DENABLE_WALLET=OFF"
(https://github.com/bitcoin/bitcoin/issues/32347)
```bash
make -C depends/
cmake -B build --toolchain /root/bitcoin/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DENABLE_WALLET=OFF
cmake --build build
./build/test/functional/test_runner.py
<snip>
wallet_watchonly.py --legacy-wallet | ○ Skipped | 1 s
wallet_watchonly.py --usecli --legacy-wallet | ○ Skipped | 0 s
feature_rbf.py | ✖ Failed | 10 s
interface_bitcoin_cli.py --legacy-wa
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2059950855)
(We had a call and I will experiment with passing down the full `sys.argv` to the child processes (with some tweaking for tmpdir). Will also see if I can add a test-class which intentionally exercises the failure case of `...count("Traceback")) != 1` and other checks in `_verify_startup_failure` without making things too convoluted, as it will ensure the test tester is robust going forward).

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