💬 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)

📝 romanz opened a pull request: "http: Use 'starts_with' for matching URI prefix"
(https://github.com/bitcoin/bitcoin/pull/30868)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 maflcko commented on pull request "http: Use 'starts_with' for matching URI prefix":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2342686200)
If there is support for this change, it may be better to use and enable https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-starts-ends-with.html . Otherwise, follow-up pull requests may be created for each instance individually. Also, newly introduced code may re-introduce new instances.

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2342749010)
I successfully tested this code as part https://github.com/Sjors/bitcoin/pull/48#issuecomment-2342745012

💬 maflcko commented on pull request "build: Fix `ENABLE_WALLET` option":
(https://github.com/bitcoin/bitcoin/pull/30867#issuecomment-2342790419)
review ACK 0037d53d1a21ec8a5a97a83ab716d68030446021

Just noting that the test config will be a bit confusing, albeit harmless:

```
$ grep -C1 USE_SQLITE ./bld-cmake/test/config.ini
#ENABLE_WALLET=true
USE_SQLITE=true
#USE_BDB=true

💬 maflcko commented on pull request "ci: Post CMake-migration fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30841#issuecomment-2342828286)
review ACK c45186ca548362b75a6640393ccf79b11ff727da

💬 maflcko commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2342833531)
review ACK b07fe666f27e2b2515d2cb5a0339512045e64761

Seems fine to always buffer everything in memory twice, because it is already buffered in memory once (as the full `hex_content`) and the data should be small (generally less than 10 MB?)