💬 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

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

> On a re-touch similar re-wording could be done here?

I don't think I would add "used" here or imply anything about the datadir here since the message is supposed to be about the config file not the datadir. The gist of the message is supposed to be: "you have two configuration files, and one is being ignored, so please merge them or get rid of the one you don't want."

Extra notes about the datadir I think are he
...

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

> I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:

Feel free to suggest the code change that you would like to see, but I think saying configuration file {initial_datadir}/bitcoin.conf from data directory {initial_datadir}" is more helpful than saying "configuration file {initial_datdir}/bitcoin.conf" because the latter tells you which configuration fil
...

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

> This line seems redundant when `allowignoredconf=1` has been provided actually:

Feel free to suggest a code change that you would like to see here.

IMO, the -allowignoredconf option is meant to provide an escape hatch for backwards compatibility when your bitcoin configuration is complicated and confusing and you don't have time to clean it up. I think implementation of the option should be as simple as possible
...

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

> nit: indent should be +2 more spaces

Thanks! Fixed this

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

> Any possibility to make the error message translatable, as for others `ConfigError` calls?

Are you suggesting actually translating this message, or just making a code change to make it easier to translations the future?

Obviously any message is translatable, but I didn't want this particular message translated because it I think it should only show up in niche cases where users have manually created multiple data
...

💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202566303)
This doesn't result in a behavior change, does it? As far as I can tell `nodestate->m_is_inbound` is initialized to the value of `pfrom.IsInboundConn()` when the `CNodeState()` is constructed -- is that understanding correct?

I'm good with avoiding the use of CNodeState where possible though, if that is the reason for this change.

If these are different somehow, then perhaps we'd want to change the logic in `IsBlockRequestedFromOutbound` to match it.