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

💬 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_r1158922586)
Why `0` and not `"none"`?

Like this one:

```cpp

case BCLog::LogFlags::ALL:
return "all";
```

💬 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_r1158916354)
`DEBUG_LOG_OPTS[0] `

Feels like this is undoing whatever convenience/readability you set up by defining `DEBUG_LOG_OPTS` in the first place. Do you think more debug args will be added in the future? Or can we just use

`args.IsArgSet("-debug") || args.IsArgSet("-debugexclude")` ... etc

💬 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_r1158925769)
GREAT

💬 mzumsande commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498015461)
I think that the data race is not the root cause, but happens as a result of the absolute timeout https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L35
not being sufficient, because the test environment is too slow for whatever reason.
In this case, the `BOOST_REQUIRE` in https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L38 fails, and the index isn't terminated correctly (by wa
...

📝 willcl-ark opened a pull request: "Deprecate and remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) `mempool` P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

It was originally introduced with a protocol version bump from 60001 to 60002 and identification of a node willing to provide this service was based on this new protocol version and the advertisment of `NODE_NETWORK`.

S
...

💬 mzumsande commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498111803)
Hmm, the existing timeout of 10 seconds seems plenty for the sync process, which takes just a few milliseconds on my computer.
@fanquake would a higher timeout fix this in your environment? Are you doing some extreme parallelization or someting, such that the test is simply that slow?
Or is there possibly some other reason that prevents the Index sync from finishing on aarch64, so that bumping the timeout wouldn't help?

💬 dergoegge commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1498115249)
Concept ACK

(I would like to see us do the same for BIP37 at some point)