👍 hebasto approved a pull request: "build: disable boost multi index safe mode in debug mode"
(https://github.com/bitcoin/bitcoin/pull/27724#pullrequestreview-1439642762)
ACK 59c89447499bd9d6202269879555b8bc37373aa2

💬 hebasto commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#discussion_r1202283522)
nit: Could split the long line?

💬 willcl-ark commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#discussion_r1202287841)
Sure, will do if I touch again :)

I did think about it at the time, but some CI tasks seem to be split on ~100 chars, and some just have long lines. So I just left this one as "long" (and made it longer).

💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559296260)
I think something like #19434 that uses `evhttp_connection_set_closecb` is needed to avoid this

💬 hebasto commented on pull request "depends: remove redundant stdlib option":
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1559341906)
> This should be a no-op.

I cannot confirm it, at least for `x86_64-apple-darwin`
```
$ git fetch origin pull/27721/head
$ git checkout FETCH_HEAD
$ make -C depends clean-all
$ make -C depends HOST=x86_64-apple-darwin
$ find depends/built/x86_64-apple-darwin -name '*.hash' | sort | xargs cat
afc3a7c266402a88b9c2bbbaa11a4dfceca5b1d4d33290080b9c31c8f5c65d25 bdb-4.8.30-d31fb2c14af.tar.gz
460b8455718a04f7d25d84179149c2d9b98b3318eedb3f89f50f93b6cff3d8ec boost-1.81.0-6596e037cc7.tar.gz
7
...

💬 theStack commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559453982)
Thanks for reporting! I observed this issue sporadically, but were never able to reproduce it until now. With the help of your description (_"... interrupting bitcoin-cli before it has received the reply ..."_) I found it's actually quite trivial at least on my machine:

1. startup `bitcoind -debug=http` (and any other desired flags; note that the hangup also happens without the debug flag, but this allows us to see where it's hanging)
2. execute `bitcoin-cli waitforblockheight 1234567` (or a
...

💬 hebasto commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1559478702)
Retracting my ACK, as `FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_msan.sh" ./ci/test_run_all.sh` fails locally for me.

💬 hebasto commented on pull request "depends: remove redundant stdlib option":
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1559505920)
> > This should be a no-op.
>
> I cannot confirm it...

Well, `diffoscope` shows only diffs in object files timestamps.

💬 MarcoFalke commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1559553912)
> Retracting my ACK, as FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_msan.sh" ./ci/test_run_all.sh fails locally for me.

Seems unrelated, as this is failing on master. I agree it should be fixed, but I don't see why something unrelated should be holding back this pull.

💬 willcl-ark commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1559577176)
Perhaps I can drop the fuzz CPPFLAGS (introduced in this pull) for now, so that nothing is broken after this commit?

```diff
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..35a0de8034 100755
--- a/ci/test/00_setup_env_native_fuzz_with_msan.sh
+++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh
@@ -17,7 +17,7 @@ export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev cmake"
# BDB generates false-positives and will
...

💬 MarcoFalke commented on pull request "build: disable boost multi index safe mode in debug mode":
(https://github.com/bitcoin/bitcoin/pull/27724#issuecomment-1559600221)
> ... so that nothing is broken after this commit?

Again, this is unrelated, because the issue happens in `master`. See also https://cirrus-ci.com/task/4517884630663168?logs=ci#L4910

🤔 furszy reviewed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1439888882)
Looking much better, thanks for applying the suggestions 👌🏼.

Left few comments, it's getting closer.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202468603)
In ad5642ae:

This two declarations can be removed. Can just make the definition static.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202477990)
In ad5642ae:

Can remove the "\n" separator by providing the `command` variable instead of the `executableCommand` string at line 428. The code adds the "\n" few lines before calling `executeConsoleOnlyCommand`.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202493790)
In ad5642ae:

Wrong Indentation here.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202482937)
In ad5642ae:

Can remove the conditional statement entirely and always return true.

1) Functions are mapped in a "command name" -> function map structure. The code will never reach this point If the command wouldn't be a `help-console`.

2) The `!parsed_command.empty()` statement is not needed. It will never be empty at this point, the parent dispatcher function already checks for emptiness.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202510683)
In 411b1da4:

Similar to the one above, The `parsed_command[0] == "help" && parsed_command[1] == "generate"` statement is not needed. The code wouldn't reach this point if the command wouldn't be a `help generate` command.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202507142)
In 411b1da4:

The `!parsed_command.empty()` is not needed. The dispatcher function already checks for emptiness.
Also, it's duplicated in both conditional branches.

Same for the `parsed_command[0] == "generate"`. The code wouldn't reach this point if the command wouldn't be a `generate`.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202498803)
In ad5642ae:

I think that would be cleaner to change the if/else with a:

```c++
const std::vector<std::string> parsed_command{parseHelper(command)};
if (parsed_command.empty()) return false;

// Iterate over m_method_map and execute the associated method if the beggining of parsed_command matches the key
etc..
```

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1202515027)
In ad5642a:

nit: instead of passing the wallet model pointer, could pass by reference. Always safer than passing pointers around.