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

🤔 vostrnad reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2838259912)
utACK 35bcd8eed38130445aef5ebe217ab42248fa6f18

All nits that can be done in a follow-up.

💬 vostrnad commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087716015)
nit
```suggestion
data_len = int(MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR) - tx.get_vsize() - 5 - 4 # -5 for PUSHDATA4 and -4 for script size
```

💬 vostrnad commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087715751)
nit: "scriptPubKey" instead of "script" is needlessly specific when we're already talking about outputs.
```suggestion
- Multiple data carrier (OP_RETURN) outputs in a transaction are now permitted for relay and mining. The `-datacarriersize` limit applies to the aggregate size of the scripts across all such outputs in a transaction, not including the script size prefix itself. (#32406)
```

💬 vostrnad commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087716118)
nit: This might be cleaner with `tx.get_weight`, avoids the division by 4. (same on line 373)

💬 vostrnad commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087716286)
nit: These two comment lines can now be merged.