💬 maflcko commented on pull request "cmake: fix a few likely documentation typos":
(https://github.com/bitcoin/bitcoin/pull/30734#issuecomment-2315350961)
The correct prefix for typo fixes is `doc`, not `cmake`, because some of them are not cmake related.


lgtm ACK 7ee5c3c5b2fb477a283df8861e28005ef514bd20

💬 maflcko commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315355414)
Sure, happy to push any other alternative, if someone sends me a diff that compiles. Also, I am happy to review an alternative pull request.

📝 hodlinator opened a pull request: "test: Fix RANDOM_CTX_SEED use with parallel tests"
(https://github.com/bitcoin/bitcoin/pull/30737)
To fix the issue with test directories clashing when running in parallel with the env var `RANDOM_CTX_SEED` set, we now mix in the test name.

Issue was introduced by 97e16f57042cab07e5e73f6bed19feec2006e4f7, and comment for `g_insecure_rand_ctx_temp_path` became invalid with that commit. Make the determinism clear through removing the no longer useful `g_insecure_rand_ctx_temp_path`.

Fixes #30696.

💬 hodlinator commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2315375003)
Can be verified through running
```
RANDOM_CTX_SEED=21 make -j10 check
```

📝 fanquake opened a pull request: "doc: fixup macOS build docs for CMake"
(https://github.com/bitcoin/bitcoin/pull/30738)
Some minor corrections.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2315385164)
Rebased for cmake

💬 maflcko commented on pull request "doc: fixup macOS build docs for CMake":
(https://github.com/bitcoin/bitcoin/pull/30738#issuecomment-2315399754)
lgtm ACK 3c53e59dcf82d248427b4e329bfead0d2773cef3

👍 TheCharlatan approved a pull request: "doc: fixup macOS build docs for CMake"
(https://github.com/bitcoin/bitcoin/pull/30738#pullrequestreview-2266376419)
ACK 3c53e59dcf82d248427b4e329bfead0d2773cef3

👋 instagibbs's pull request is ready for review: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592)

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2315409258)
rebased for cmake and un-drafted since we have split off 28.X branch

💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2315435670)
> Actually, doesn't the fact that the Rw methods on both Node and Chain interfaces directly use ArgsManager violate that principle too?

This is ok because of the way multiprocess code works. When node and wallet code are running in the same process, and the wallet calls a virtual `interfaces::Chain` method, this just invokes the `node::ChainImpl` implementation of that method in `src/node/interfaces.cpp`. But when node and wallet code are running in different processes, and wallet code calls
...

💬 TheCharlatan commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315456789)
Does this work for you?
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 32db4189d4..f292830798 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -74 +73,0 @@ enable_language(CXX)
-set(CMAKE_CXX_STANDARD 20)
@@ -563,0 +563 @@ target_compile_definitions(core_interface_debug INTERFACE ${DEPENDS_COMPILE_DEFI
+target_compile_features(core_interface INTERFACE cxx_std_20)
```
Maybe @hebasto has a suggestion for also printing it in the summary.

👍 hebasto approved a pull request: "doc: fixup macOS build docs for CMake"
(https://github.com/bitcoin/bitcoin/pull/30738#pullrequestreview-2266429454)
ACK 3c53e59dcf82d248427b4e329bfead0d2773cef3.

📝 fanquake opened a pull request: "doc: update dev note examples for CMake"
(https://github.com/bitcoin/bitcoin/pull/30739)
Update the examples in the developer notes to work with CMake.

💬 stickies-v commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2315470575)
> This is ok because of the way multiprocess code works ... there is not more than one process accessing the settings file.

Thank you, that explanation actually clears up a lot, appreciate the thoughtful response, as always.

> and maybe those could be moved from ChainImpl to ArgsManager so the ArgsManager interface is more complete and glue code is simpler.

and it would also allow code re-use across both interfaces.

🚀 glozow merged a pull request: "doc: fixup macOS build docs for CMake"
(https://github.com/bitcoin/bitcoin/pull/30738)

💬 maflcko commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315498877)
> Maybe @hebasto has a suggestion for also printing it in the summary.

IIUC `-std=value` should always be printed in the summary (before and after)

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1734804944)
IIUC this would at least give more coverage to `AddTxAnnouncement` getting deeper during new orphan processing

👍 instagibbs approved a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2266475743)
reACK 2b387a156a1021ad782dc4de2fd34d5b5b81698a

via `git range-diff master 6f6fc1464ad2f2981e7c3ff5d6492f60f89a6504 2b387a156a1021ad782dc4de2fd34d5b5b81698a`

only non-cmake changes I see were reordering the class renaming, and the added `ConnectedPeer` in unit tests.

💬 fanquake commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2315518298)
Rebased, and switched the changes to CMake.