💬 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

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155615459)
they don't have to be active
```diff
- // Find candidate active xpubs
+ // Find candidate xpubs
```

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155623853)
why do we need to erase it?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1155626611)
This requires compatibility between `WALLETDESCRIPTORCKEY` and `HDCKEY`. Should we document it somewhere?

💬 TheCharlatan commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1493933961)
> I'm having trouble understanding the relationship between this PR and that issue, am I missing something?

The CI was previously failing on the functional tests due to that issue.

💬 fanquake commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1493961444)
> If it doesn't look right I can fix it. Appreciate the help.

@jessebarton There are currently three commits here, when it should be (squashed to) one.