💬 arnabnandikgp commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1493382262)
I am not able to understand exactly what this issue is trying to resolve..I have realised that the python script talked about here generates man-pages for each binary in the src/doc/man directory..but I am not able to understand what is meant by the documentation of arguments of build parameters...can anyone please explain me and clear my confusion regarding the same.

💬 vasild commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1155339537)
If this assert is hit, it means that somebody has added a new entry in `enum Network`, has forgotten to update this function, has ignored the compiler warning and has ignored the CI failure. Or a memory corruption has occurred and `CNetAddr::m_net` has been overwritten with random bytes. I think `assert(false)` is the appropriate response in both cases.

💬 jessebarton commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1493388398)
This was my first time doing a squash commit. If it doesn't look right I can fix it. Appreciate the help.

💬 hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1493388491)
Updated up to [pr25797.50](https://github.com/hebasto/bitcoin/commits/pr25797.50):

- rebased on the current [cmake-staging](https://github.com/hebasto/bitcoin/tree/cmake-staging) branch
- some CI regressions need to be fixed

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1155345991)
I really disagree on this point. We have had many examples over the years of asserts that people have added to our networking code for reasons that seem justifiable (along the lines of the reasons you've given), only to discover later that (usually due to an unexpected combination of code changes elsewhere) we ended up opening up a remote crash bug in deployed software that can take down the network -- in situations where we could easily have written code more defensively so that our software wo
...

💬 martinus commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1155347362)
I'd explicitly set the move operations to `= delete`. I think `std::mutex` is not movable so these will be deleted anyways, but it feels safer to just mark them as deleted too. There will be threads running throughout the lifetime of `CCheckQueue`, so moving the object while something might be going on or threads waiting on a lock wouldn't be safe

💬 martinus commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1155341232)
nit: might want to add `m_worker_threads.reserve(worker_threads_num);` before the loop

💬 martinus commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155351001)
Macro magic to the rescue! This works for me:

```patch
diff --git a/src/test/util_trace_tests.cpp b/src/test/util_trace_tests.cpp
index 6e3e846b4f..952b9cc6e4 100644
--- a/src/test/util_trace_tests.cpp
+++ b/src/test/util_trace_tests.cpp
@@ -18,7 +18,7 @@ BOOST_FIXTURE_TEST_SUITE(util_trace_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(test_tracepoint_zero_args)
{
- TRACEPOINT0(test, zero_args);
+ TRACEPOINT(test, zero_args);
BOOST_CHECK(true);
}

diff --git a/src/ut
...

💬 vasild commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1155351861)
I think this method belongs to `CConnman`, something like:

```cpp
bool CConnman::AllowOutboundByNetgroup(const std::set<std::vector<unsigned char>>& connected_netgroups,
const CAddress& addr)
{
return connected_netgroups.count(m_netgroupman.GetGroup(addr)) == 0 ||
(gArgs.GetArgs("-onlynet").size() == 1 && IsReachable(addr.GetNetwork()) &&
(addr.IsTor() || addr.IsI2P() || addr.IsCJDNS()));
}
...

💬 hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1493435688)
> * some CI regressions need to be fixed

The "previous releases" CI task has been fixed.

💬 hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1493484482)
> * some CI regressions need to be fixed

The "multiprocess" CI task has been fixed.

💬 amitiuttarwar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1493586946)
RE `fCountFailure` bool:
@mzumsande
> to determine the fCountFailure bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (nAttempts). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave setConnected unchanged as sdaftuar suggested?

@sdaftuar
> I think that bit of code that we're using in our OpenNetworkConnection() call is really trying to determine how many open connection
...

💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1493661021)
Added followup commit for tapscript(+ suggestion from @vostrnad )!

💬 martinus commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768)
Based on @willcl-ark's comment https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1480997053 here is a version that works without any warnings in clang and gcc:

```patch
diff --git a/src/test/util_trace_tests.cpp b/src/test/util_trace_tests.cpp
index 6e3e846b4f..952b9cc6e4 100644
--- a/src/test/util_trace_tests.cpp
+++ b/src/test/util_trace_tests.cpp
@@ -18,7 +18,7 @@ BOOST_FIXTURE_TEST_SUITE(util_trace_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(test_tracepoint_zero_ar
...

📝 martinus opened a pull request: "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings"
(https://github.com/bitcoin/bitcoin/pull/27401)
Fixes #26916 by disabling the warning `-Wgnu-zero-variadic-macro-arguments` when clang is used as the compiler.

Also see the comments
* Proposed changes in the bug https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1480997053
* Proposed changes when moving to a variadic maro: https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155586506)
better
```diff
- if (LEGACY_OUTPUT_TYPES.count(output_type.value()) == 0) {
+ if (tmpl.count(output_type.value()) == 0) {
```
or
```diff
- if (LEGACY_OUTPUT_TYPES.count(output_type.value()) == 0) {
+ if (descs_keys[xpub].first.count(output_type.value()) == 0) {
```

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155604123)
naming nit: `desc_keys` and `descs_keys` are very easy to mix

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155609607)
Do we want to check that we have exactly one `internal=true` and one `internal=false` descriptors?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155610688)
nit: maybe good time to add typedef. I had a very hard time reading the signature of the fuction

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155618170)
Could clarify comment:
For all the candidate xpubs add corresponding private key if available