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

💬 willcl-ark commented on issue "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)":
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-1498130708)
> But in that case it shouldn't be 500, more like 400 Bad Request - it's not the user that failed, it's the client that requested a non-existing block. But again, it's only a theory.
This looks to me like it will be fixed by https://github.com/bitcoin/bitcoin/pull/27101 if requesting using json 2.0 spec, where we will return status code `200` (and only an `error` field). @pinheadmz might be able to confirm?

💬 RandyMcMillan commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1498134466)
ACK ed4a833

🤔 EthanHeilman reviewed a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1159011140)
done

💬 jonatack commented on pull request "test: create random and coins utils, add amount helper, dedupe add_coin":
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1498148650)
> setup_common can't use any of the random functions without creating a circular dependency

Addressed with #27425.

💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498153284)
RE https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1373289140

> 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/comm
...

💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498165963)
Updated c02e451882bb1d220b944af3b444d4b529ac351a -> de2546e672d0a2103228d777b35bfa6aab4881ee ([splitSystemArgs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_1) -> [splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_1..splitSystemArgs_2)):
* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412) adding a missing copyright
...

🤔 mzumsande reviewed a pull request: "Deprecate and remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
> or anybody still actively using this message to find out more about their use-cases.

how about a mailing list post to reach a wider audience?

💬 willcl-ark commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1498202324)
@kylezs Could you program your test to use the `--fallbackfee` if it recieves the error in reponse, or as Antoine reponded to you there fill up your test mempool so that `estimatesmartfee` works as intended?

Accessing `--fallbackfee` from within the fee estimation rpc involves importing a lot of wallet code which woudl be preferable to avoid...