🤔 glozow reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2838089123)
ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef

✅ glozow closed an issue: "Bitcoin is returning higher fees for 36 block window than 2 block window (on testnet)"
(https://github.com/bitcoin/bitcoin/issues/11800)

🚀 glozow merged a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080)

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087617462)
Done.

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2877918269)
Updated to address @TheCharlatan's nit.

🤔 stringintech reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2837977865)
I had a few more comments (sorry for the scattered feedback; didn't have the chance to review everything earlier).

💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087608225)
In commit 5908680, the description mentions: "By returning early in case of a BIP30 validation failure, an additional failure check for an invalid block reward in case of its failure is left out."

To validate my understanding: there are also other checks in `ConnectBlock` before the `!state.IsValid()` check that we may have skipped because of the early return when `SpendBlock` fails - including transaction input validation, script verification, fee range checks, ...; Is that correct?

It ma
...

💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087539901)
Looks like we have an early return in `GetTransactionSigOpCost` for coinbase tx and no use for `coins` span; so maybe we could just pass an empty span here?
```suggestion
nSigOpsCost += GetTransactionSigOpCost<const Coin>(tx, {}, flags);
```

💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087577718)
Do we need to update the [documentation](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/consensus/tx_verify.h#L24:L31) for `CheckTxInputs` now that the input existence check is removed from it?

📝 maflcko opened a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488)
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.

This could be fixed by fixing the initialization order.

However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.

So instead fix it by constructing a fresh wallet for each fuzz iteration.

🤔 hodlinator reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2838013470)
Code Review 4e1aae19512df82af584a064640c2143c5c5fa4f

Seems like a slight fix would be good in the NSI script, see inline comment.

Nice simplification of `ExecVp` since [previous review](https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821205668).

Tested on NixOS.

### Tested redirection of stderr/stdout

```
./build/bin/bitcoin node > foo
./build/bin/bitcoin node --nonexistentarg 2> err
```
"foo" stdout file contains expected log output. Confirmed by changing bit
...

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087562615)
Would be nice to merge exec.h/cpp into subprocess.h due to similar functionality. But I don't know what happens to copyright in that case, so might be better to avoid for that reason.

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087618429)
nit: Could document requirements in header:
```C++
//! Cross-platform wrapper for POSIX execvp function.
//! @param argv Needs to end with nullptr element, in line with execvp.
int ExecVp(const char* file, char* const argv[]);
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087636740)
It appears we should also be deleting this in the uninstall section further down, right after:
https://github.com/bitcoin/bitcoin/blob/3eb0d327153f61ef2121a4b261b024f6ee1f3398/share/setup.nsi.in#L133

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087585166)
Should this include util/exec.h?

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087629089)
Might be nice to add a rationale for `-named` in the commit message of fc47f776fbafaaf9062e6b4dc29ba6e42e390d2a as it isn't discussed in the linked issue.

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087583779)
Might be clearer to return `std::nullopt` upon failure? Feels a bit sloppy currently, but maybe okay for a glorified shell script as you wrote elsewhere: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920888966

✅ l0rinc closed a pull request: "doc: document workaround and fallback for macOS fuzzing"
(https://github.com/bitcoin/bitcoin/pull/32084)

💬 maflcko commented on pull request "fuzz: Properly setup wallet in wallet_fees target":
(https://github.com/bitcoin/bitcoin/pull/32488#issuecomment-2877970793)
Can be tested via:

```diff
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index e32b8c7272..ea67a48e9e 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -468,6 +468,7 @@ public:

~CWallet()
{
+ chain().relayMinFee();
// Should not have slots connected at this point.
assert(NotifyUnload.empty());
}
```

and:

```
rm -rf ./bld-cmake && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-std=c++23 -O3 -g2' -DAPPEND_CFLAGS='-O3 -g2'
...

👍 brunoerg approved a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488#pullrequestreview-2838169693)
code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d

Nice! I think the initial idea was to have a global wallet because everything the target was supposed to do shouldn't affect it. But surely it's better to not have it globally and avoid any possible issue.