🤔 pablomartin4btc reviewed a pull request: "Decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2351869077)
tACK 002b792b9a85100d89e47706c29cf1fd355d9727

Tested the rpc console with and without `-DENABLE_WALLET`. I think the refactor makes sense as the rpc just needs the wallet name for the related commands.

Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the `settings.json` file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn't correspond with the one in the main win
...

💬 pablomartin4btc commented on pull request "Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#discussion_r1790172791)
nit: describe wallet_name as a param[in] (wallet_model wasn't there either)
```suggestion
* @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
* @param[in] wallet_mame ...
```

💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396911373)
> It is just two:

It's not though, because `gcc compiler + gcov/lcov` already encompasses more than 1 thing. i.e supporting different versions of gcov/lcov across releases.

🤔 hebasto reviewed a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2351928155)
Post-merge ACK 56aad83307e46983a397236bd0959e634207f83e.

💬 andrewtoth commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2396914856)
> Does it still happen after https://github.com/romanz/electrs/pull/1091 ?

That is for a different fork of electrs. For the mempool/electrs, you can see in the original logs in #31039 that it is already waiting for mempool to load before syncing.

💬 maflcko commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2396926622)
> nit: might be better to use `static_cast` instead of the constructor-style cast.

Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.

⚠️ fanquake opened an issue: "build: macOS fuzz instructions broken on 15.x"
(https://github.com/bitcoin/bitcoin/issues/31049)
Testing master at 62e4516722115c2d5aeb6c197abc73ca7c078b23 and the fuzzing.md instructions:
```bash
cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
cmake --build build_fuzz
<snip>
[100%] Linking CXX executable fuzz
ld: multiple errors: invalid r_symbolnum=1 in '/Users/michael/fanquake-bitcoin/build_fuzz/src/test/fuzz/CMakeFiles/fuzz.d
...

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1790246276)
Good catch! Just to make sure I ran it 100 times and on the 90th attempt it failed, I removed the assert in [33e4dc8](https://github.com/bitcoin/bitcoin/pull/31040/commits/33e4dc834f433a51d5a4a8ffa7ce81a98ac35b7a)

💬 Sjors commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2396969984)
Concept ACK. See https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382352367

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2396973132)
`54f4f3b05e...f9b2eaf96c`: rebase due to conflicts

💬 ismaelsadeeq commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2397020325)
> Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.

Agreed I am talking about changing `size_t(int64_t(stack.size())` to `static_cast<size_t>(static_cast<int64_t>(stack.size())` same for `altstack` ?

💬 maflcko commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2397035722)
You'll also have to remove `doc/build-osx.md:54:For macOS 11 (Big Sur) and 12 (Monterey) you need to install a more recent version of llvm.`, no?

💬 hebasto commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2397047130)
> You'll also have to remove `doc/build-osx.md:54:For macOS 11 (Big Sur) and 12 (Monterey) you need to install a more recent version of llvm.`, no?

Thanks! Updated.

💬 maflcko commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2397050476)
I am happy to change code, if there is a reason. However, the dev notes recommend this style, so for now I will not address style nits one way or another. If you want to change or discuss the dev notes, a separate issue or pull request may be better.

💬 maflcko commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#discussion_r1790324492)
style nit: Seems fine, but I preferred the previous approach where all fuzz targets are registered with the same macro. This should be possible by guarding the `FuzzFrameworkRegisterTarget` behind a `if constexpr (opts.require_build_for_fuzzing and not build_for_fuzzing)`.


This should also make https://github.com/bitcoin/bitcoin/pull/30882 easier to implement, because it doesn't have to deal with two macros.

💬 maflcko commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2397086472)
Does it also happen with `llvm@16`, instead of the current version?

📝 marcofleon converted_to_draft a pull request: "fuzz: Add fuzz-only build mode option for targets"
(https://github.com/bitcoin/bitcoin/pull/31028)
Addresses https://github.com/bitcoin/bitcoin/issues/30950.

Any targets that require BUILD_ON_FUZZING=ON (currently only `p2p_headers_presync`) to work properly can now set `require_build_for_fuzzing` as an option. If BUILD_FOR_FUZZING is not on and you try to run a target that has this option, then there's a message printed upon exit.

With this change, the CI will be able to run the fuzz test runner without any timeouts/failures.

💬 maflcko commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2397104680)
I see. It would be good to know what the batch request is looking like. Also, it would be good to confirm that this works on 27.x and is broken on 28.x. Also, it would be good to confirm that the system has enough memory to fit the whole batch request response (as json) into memory.

👍 theuni approved a pull request: "ci: Add missing -DWERROR=ON to test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/31045#pullrequestreview-2352144845)
utACK fa1cffacae61264ac89e30a069ba093495cb3216.

Nice catch on the Werror.

A quick search confirms that nproc and hw.logicalcpu should be the same value.

💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2397117284)
`-v2transport=0` should be added to the bitcoind args used for netdriver fuzzing. Otherwise, it doesn't seem like honggfuzz ever fuzzes anything besides the v2 handshake.

Also playing around with this and reading the [netdriver post](https://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html) (i couldn't find any other docs on it) I'm not sure if this will ever even get past the version handshake? So I'm wondering how useful this tool really is for us.