👍 ryanofsky approved a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419)
undefinedCode review ACK c02e451882bb1d220b944af3b444d4b529ac351a

Just an option to consider, but I think could be good to move `GetConfigFile`, `GetConfigOptions`, `ArgsManager::ReadConfigStream`, and `ArgsManager::ReadConfigFiles` functions to `src/common/config.cpp` instead of `src/common/args.cpp`, since in longer run, it would be nice if it would be nice if there were a `src/common/settings.cpp` file to parse the settings file, `src/common/config.cpp` to parse the config file, and `src/common/arg
...

💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158754838)
undefinedIn commit "refactor: Extract common/args from util/system" (c02e451882bb1d220b944af3b444d4b529ac351a)

Would be good to drop the coment above line 7 "Server/client environment: argument handling, config file parsing"

💬 mzumsande commented on pull request "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1497845534)
undefined0577bd6d1bcbbc808fd20d7a368a562799ceccc3 to 974140f9e721740f857b45d10d7dbab62fdbbe53: rebased due to conflict with #25781

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#discussion_r1158814308)
undefinedYeah the warning is the same, but in the GUI we have these related-but-different classes `RecentRequestEntry` for one view and `AddressTableEntry` for another. I don't see a great common file in `qt/` to declare warning strings, so I'm open to suggestions or maybe I should just start a new file?

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1497872600)
undefinedThanks for all the style notes @jonatack I addressed all your comments including adding a new struct for `ReceiveRequest` and renaming/qualifying the new method to `[[nodiscard]] GetAddressWarnings()`. Also addresed your nit in https://github.com/bitcoin/bitcoin/pull/27216 and rebased both PRs on master.

💬 pinheadmz commented on pull request "Add warnings for non-active addresses in receive tab and address book":
(https://github.com/bitcoin-core/gui/pull/723#issuecomment-1497880274)
undefinedI also moved the warnings column to the right, after date. I think we could just remove the title "warnings" and that column would narrow down to 1 character ...?


👍 hebasto approved a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
undefinedACK c40c9edf9266bac0b3553a3d6c5b97f94c34312d

💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1158836947)
undefinedThis baseline implies package versions as they were at `2022.02.23` tag, which means older ones than they are in our master branch for now. For details, see https://cirrus-ci.com/task/4695704203952128.

Suggesting to update it up to the `2023.01.09` tag:
```suggestion
"builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996",
```

📝 jonatack opened a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425)
and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!)

Also update the remaining rand calls in the tests to use the common `util/random` helpers.

💬 furszy commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1497934186)
> > I never said the opposite. Not sure why the "rush" term is being used here really. I haven't said anything like "this is great, we must have it now".. (nor here nor anywhere in the repository).
>
> I'm sorry didn't mean to put words in your mouth. It seemed rushed to me because you are going with an "easier" solution here rather than the more complex but (imo) worth while long term design.

Well, this PR didn't have any long term design goal discussion prior to your arrival.
I'm happy
...

💬 pinheadmz commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1158870887)
Why do you set this flag for the entire test? Since the `warnings` is the new thing going forward, wouldn't it make more sense to write all the tests with only that field by default? And just add one test case with the flag to check `warning` was also included?

💬 pinheadmz commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1158867456)
Is this the best way to test for this? Seems like it could be broken by someone using a word in a future doc update. You can't get `bitcoind --version` or something for the back-compat test?

💬 pinheadmz commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1158883099)
> so no need to encumber wallet.h and wallet.cpp with it

I'm not sure I totally agree with this, but just because all the flags are defined here already. Like, you'd need to import this header file to use `WALLET_FLAG_AVOID_REUSE` anyway, why not keep the caveats right next to their associated flags?

👋 pinheadmz's pull request is ready for review: "Add warnings for non-active addresses in receive tab and address book"
(https://github.com/bitcoin-core/gui/pull/723)

💬 fanquake commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1497996395)
Concept ACK - will test

💬 fanquake commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1497996800)
Concept ACK

💬 mzumsande commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1497996893)
Maybe also add a short explanation before the `OpenNetworkConnection()` call about the rationale for what we pass for `fCountFailure` from https://github.com/bitcoin/bitcoin/pull/8065 (Good find @amitiuttarwar!), i.e. the scenario where we can't connect to the internet but still may have a local connection. This was not obvious at all to me.

🤔 pinheadmz reviewed a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231)
concept ACK
github code review, will fine-tooth comb review later this week.
Played with build locally and tried out examples from new help message. A few questions...

💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1158920648)
Recommend putting "all" and "none" in quotes to be super-duper explicit

💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1158922726)
not going to add `"none"` here?