👍 tdb3 approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2293665953)
ACK a1e737ef089d407256428ea29be20f46bbea71fe

👍 ryanofsky approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2293686068)
Code review ACK 48b67d8294eb62c5371e7dbc30b7cf811fb7e51c. This looks great! PR is much simpler now, and it is nice to see the GCC workaround expanded and documented so well.

Would suggest a few changes, implemented in diff below:

- Replacing `PushDataSize` `size_t` parameter with `uint32_t` parameter to document fact that it can't accept larger sizes, and to call `WriteLE32` without a type conversion.
- Replacing `PushData` `const value_type* data, size_t size` parameters with `std::span<
...

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752677734)
Done. Updated.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752677951)
Done. Updated.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1752678000)
Done. Updated.

💬 fjahr commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2341951885)
> > How about adding an instruction for testing assumeutxo mainnet parameters?
>
> Not related to the testing guide, but it could also be mentioned in the release notes that assumeutxo mainnet params are available now.

I have added it to the [release notes draft](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft).

👍 tdb3 approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2293721430)
ACK 082779d6062c5b72f3497bb864e4dbb4373a3a4c

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752696755)
> making implementation details verbose to use is a feature to avoid developers from accidentally using them in real code

I can't immediately see a use case for this function, but is there a reason we'd want to actively discourage this function from being used? It seems harmless to me, but if there is, perhaps it'd be good to add a brief docstring to the function to describe that?

> The verbosity in tests is a bit annoying, but seems acceptable to me.

I agree.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752698600)
> Given that they are not used,

Yup, I agree with the simplified approach. I didn't like the half/half of quite a of bit of complexity without being complete, this makes more sense. Can be marked as resolved, thanks!

🤔 TheCharlatan reviewed a pull request: "build: Use CMake's default permissions in macOS `deploy` target"
(https://github.com/bitcoin/bitcoin/pull/30838#pullrequestreview-2293751960)
Guix build (aarch64):
```
8eeb195889318631934b31746dcbd04b26c2312e2aa29dbd7be9de9581a8b805 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/SHA256SUMS.part
05c10866f6972aa58a379935dbc4eb39a8e2abc54a27aa4fa2ce06a7443593ba guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin-5ba03e7d35e1-arm64-apple-darwin-unsigned.tar.gz
f81ec9368bf068a373ec88428becdef08533e8984ad95a3b017f7f1a8cbbb5b1 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin-5ba03e7d35e1-arm64-apple-darwin-unsigned.z
...

🤔 furszy reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2293758551)
> > AssumeUTXO nodes prioritize tip synchronization first, meaning they advertise
> > knowledge of all blocks posterior to the snapshot base block hash.
>
> This is confusing to me - nodes advertise `NODE_NETWORK` (all historical blocks prior to tip) or `NODE_NETWORK_LIMITED` (last 288 from tip). They also communicate their current tip with peers, but it's not part of the advertisement. So this includes (amongst others) blocks **prior** to the snapshot block hash, not posterior.

Yeah ok.
...

💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2342010162)
ACK 59028de422b

👍 theStack approved a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2293772589)
re-ACK 082779d6062c5b72f3497bb864e4dbb4373a3a4c

📝 hebasto opened a pull request: "build: Fix `ENABLE_WALLET` option"
(https://github.com/bitcoin/bitcoin/pull/30867)
The removed commands were left over from the transition from autodetection to explicit options in the CMake staging branch. These commands prevented the `-DENABLE_WALLET=OFF` option from being work properly when building with depends.

How to test:
```
$ make -C depends NO_QT=1
```

On the master branch @ c66c68345efb0bb3d5613ebac703cde779fa0f01:
```
$ rm -rf build && cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DENABLE_WALLET=OFF
< snip >
Optional features:
...

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#issuecomment-2342178439)
> Replacing PushDataSize size_t parameter with uint32_t parameter

Good observation, thanks.
I was being overly cautious with modifying that code.

> Making the char operator call the std::byte operator to make it smaller, deduplicate code, and make it obvious the operators are equivalent.

My goal was to avoid extra casting and wrapping, especially in performance-critical areas like this.

Right now, we avoid intermediary vectors, but with this change, we'd wrap the data in a `std::spa
...

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752854915)
I think it makes sense to throw an error in this situation. Although it's not necessarily due to disk corruption, it would also be possible that the block/undo data is pruning event right after the blockindex check (`cs_main` is released) and before the `UndoReadFromDisk` call.

We also have a functional test for just this situation that wouldn't work anymore ([here](https://github.com/bitcoin/bitcoin/blob/c66c68345efb0bb3d5613ebac703cde779fa0f01/test/functional/rpc_blockchain.py#L615-L628)) b
...

💬 kevkevinpal commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#issuecomment-2342307179)
ACK [5c80192](https://github.com/bitcoin/bitcoin/pull/30847/commits/5c80192ff6b982ee3a75be4142fe942b8206f2cd)

`cmake -B build -DENABLE_EXTERNAL_SIGNER=OFF (and commented out banman_tests)`
`cmake --build build`
`ctest --test-dir build`

Got the following snippit
```
<snip>
Start 13: banman_tests
13/133 Test #13: banman_tests .........................***Skipped 0.01 sec
</snip>

<snip>
Start 99: system_tests
99/133 Test #99: system_tests ...............
...

💬 kevkevinpal commented on pull request "docs: Updated debug build instructions for cmake":
(https://github.com/bitcoin/bitcoin/pull/30840#issuecomment-2342354487)
ACK [0b003e1](https://github.com/bitcoin/bitcoin/pull/30840/commits/0b003e1ff7e09b56cd013b15f1998495994b7211)

Also in this parity list `--enable-debug` is replaced with `-DCMAKE_BUILD_TYPE=Debug` and defaults to `RelWithDebInfo`
https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e

🤔 glozow reviewed a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2294829330)
ACK 082779d6062c5b72f3497bb864e4dbb4373a3a4c

🚀 glozow merged a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805)