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

💬 maflcko commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087661003)
The source says that this is a constant: https://doc.rust-lang.org/src/std/hash/random.rs.html#109-111

```
pub fn new() -> DefaultHasher {

DefaultHasher(SipHasher13::new_with_keys(0, 0))

}
```

whereas the one using RandomState uses `hashmap_random_keys`

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087666516)
OK then, `RandomState` is definitely needed.

💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087687610)
OK, I don't really care. Especially since the exact people who are spamming the PR did not even answer this, I think they don't care.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2878061406)
Will push a wider update based on the BIP159 since https://github.com/bitcoin/bips/pull/1768.

📝 achow101 opened a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489)
Currently, if a user wants to use an airgapped setup, they need to manually create the watchonly wallet that will live on the online node by importing the public descriptors. This PR introduces `exportwatchonlywallet` which will create a wallet file with the public descriptors to avoid exposing the specific internals to the user. Additionally, this RPC will copy any existing labels, transactions, and wallet flags. This ensures that the exported watchonly wallet is almost entirely a copy of the o
...