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

📝 evansmj opened a pull request: "doc: Add Python install notes to build-osx.md."
(https://github.com/bitcoin/bitcoin/pull/27728)
When installing Bitcoin Core on OSX, a new developer may run into a situation where the deploy dependencies `pip3 install ds_store mac_alias` may be installed to an incorrect version of Python on the machine. I had this issue and spent much time on it. I add clarifying notes to the documentation and a recommendation to use PyEnv, as **test/functional/README.md** recommends it.

I spent a lot of time on resolving my error `NoModuleFoundError No module named 'ds_store'` due to this, and addi
...

💬 jamesob commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1559648210)
Blackbox ACK

Been running an earlier version of this branch (a9f914515df0) for 5+ days and measuring header-to-tip timings with a recent bmon addition (https://github.com/chaincodelabs/bmon/pull/27); data is here: https://gist.github.com/jamesob/b601f083d5d88fdd3432da65bf5f43ae.

This branch seems to compare well with other versions I'm running. I'm going to redeploy with HEAD and will get back with some more specific analysis.

💬 theuni commented on pull request "depends: remove redundant stdlib option":
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1559649295)
Sorry, I guess I should've said it's a behavioral no-op.

A simple test-case shows that search-paths are unchanged with or without the `-stdlib=libc++` flag:
`env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/cory/dev/llvm-project/build3/bin/clang++ --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/home/cory/dev/bitcoin2/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/cory/dev/bitcoin2/de
...

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202561710)
Hmm right, this is now completely redundant and can be removed.

🤔 ryanofsky reviewed a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1371794249)
Updated 0319de5cbedd1a8f8766cfec61151c58b3fb27ef -> eefe56967b4eb4b5144325cde4f40fc1cbde3e65 ([`pr/ignoredconf.13`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.13) -> [`pr/ignoredconf.14`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.13..pr/ignoredconf.14)) with suggestions

Thanks for the reviews!

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1157865333)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150550636

> So perhaps could go next to that in a future PR?

Sure, moved there now

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1157769058)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313450

> If you re-touch, perhaps this could read:
>
> 'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '

Thanks applied suggestion

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451370)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447

Thanks! Applied change