🤔 ismaelsadeeq reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2266591139)
code review ACK a563f41232e2dd360ee8e76f6348dd10c7f4f2a3

💬 l0rinc commented on pull request "Update documentation generation example in developer-notes.md":
(https://github.com/bitcoin/bitcoin/pull/30741#discussion_r1734877711)
Fixed, thanks!

💬 fanquake commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315645641)
> Users shouldn't be able to modify this at all,

In general this is impossible, because they can always just override the compilation flags.

💬 l0rinc commented on pull request "doc: fix a few likely documentation typos":
(https://github.com/bitcoin/bitcoin/pull/30734#issuecomment-2315655210)
> You already have a PR open to fix typos.

I tried separating them by topic (the other one fixes paths mostly, this one fixes random typos and command i found).
Do you want me to merge the two?

💬 maflcko commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315658669)
> > Users shouldn't be able to modify this at all,
>
> In general this is impossible, because they can always just override the compilation flags.

Are you sure. Locally when using `-DCMAKE_CXX_FLAGS='-std=c++17'`, it doesn't have any effect, because they are reset by cmake:

```
C++ compiler flags .................... -std=c++17 -O2 -g -std=c++20 -fPIC ...
```

I guess I can use the existing non-standard append hack instead, which users shouldn't know about to shoot themselves into t
...

👍 marcofleon approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2266609845)
Code review ACK 948238a683b6c99f4e91114aa75680c6c2d73714. Only changes since my last review are the improvements in `prevector_tests`.

💬 l0rinc commented on pull request "cmake: remove unused src_dir param from run_tests":
(https://github.com/bitcoin/bitcoin/pull/30733#issuecomment-2315664216)
Done, thank @maflcko

💬 fanquake commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#discussion_r1734893592)
Wont this break the usage as described in the readme:
```bash
BUILDDIR=$PWD/build ./contrib/devtools/gen-manpages.py
/root/ci_scratch/build/build/src/bitcoind not found or not an executable
```

💬 maflcko commented on pull request "build: Add option to use C++23 for testing":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315669459)
> I guess I can use the existing non-standard append hack instead

Force pushed that instead, so that this is a doc/ci-only change.

Other changes can be done in a follow-up, if needed.

💬 fanquake commented on pull request "ci: Use C++23 in one task":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315674441)
> it doesn't have any effect, because they are reset by cmake:

Heh, right, I forgot CMake makes a mess of this. Note that this behaviour is itself a footgun, because a user might rightly expect something like `-DCMAKE_CXX_FLAGS='-O1'` to work, but CMake will just ignore it.

💬 maflcko commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#discussion_r1734898536)
Seems fine to leave this section as-is, because no update is required?

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2315678639)
Applied the rest of the suggestions.

💬 fanquake commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2315683358)
> Applied the rest of the suggestions.

You missed this one:

> Can you update the .. commit to start with doc: instead of CI:?

✅ fanquake closed an issue: "b-msghand[4988] general protection fault"
(https://github.com/bitcoin/bitcoin/issues/30706)

💬 fanquake commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2315696038)
Closing for now. If you do see any issues in future, please open new issues, including a core dump if possible (or inspect the dump locally and post any relevant info to the issue).

💬 fanquake commented on pull request "ci: Re-add configs removed in cmake migration":
(https://github.com/bitcoin/bitcoin/pull/30740#discussion_r1734915212)
Should this job also be installing `libzmq3-dev` ?

💬 l0rinc commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#discussion_r1734917436)
> To use this tool with out-of-tree builds set `BUILDDIR`.

Does this comment still apply?
Should I change it to `builddir = os.getenv('BUILDDIR', topdir)` to default to the build dir instead of modifying the `BINARIES` paths?

💬 l0rinc commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#discussion_r1734918243)
@maflcko, can you please expand on that?

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2315713202)
> Can you update the .. commit to start with doc: instead of CI:?

done (I interpreted it backwards because of the colons)

💬 fanquake commented on pull request "ci: Re-add configs removed in cmake migration":
(https://github.com/bitcoin/bitcoin/pull/30740#issuecomment-2315714002)
> many configs were removed from the CI without explanation.

Nice catch. It's not clear why a bunch of CI configs would have been removed..