✅ achow101 closed a pull request: "Fix minor typo"
(https://github.com/bitcoin/bitcoin/pull/27102)

💬 jonatack commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107491391)
> LOCK(m); // but this will crash if m is held, so what is the point?

Ah, I see that double lock detection at runtime was added by you in 95975dd08d8f.

```diff
+++ b/src/netbase.h
@@ -73,6 +73,7 @@ public:
void Add(Network net) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
AssertLockNotHeld(m_mutex);
+ LOCK(m_mutex);
if (net != NET_UNROUTABLE && net != NET_INTERNAL) {
LOCK(m_mutex);
```
```
2023-02-15T17:48:41Z SetNetworkActive: true
2023-
...

💬 jonatack commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107505114)
> If it is really desirable to have `AssertLockNotHeld()` before every `LOCK()`

I think the general idea is to have a runtime lock assertion corresponding to every thread safety annotation, where it makes sense (cf the developer notes).

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1431795348)
yet another rebase, only dropping the "Add xoroshiro128++ PRNG" commit which has already been added in #26153 (commit 5f05b27841af0bed1b6e7de5f46ffe33e5919e4d)

💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431798320)
> Will we be OK with this PR as a "reviewing stage"? I mean, adding more commits after reviewing and ACKing of previous ones, with optional squashing of already reviewed commits, and rebasing on top of the recent changes in the current build system in the master branch.
>
> Doing in such a way we can get a reviewed CMake-based build system implementation ready to be merged at some moment in the future.

My comment above:
> I suggest that we create a staging branch/repo for reviewing and me
...

💬 achow101 commented on pull request "wallet: ensure the wallet is unlocked when needed for rescanning":
(https://github.com/bitcoin/bitcoin/pull/26347#issuecomment-1431798939)
ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af

💬 AdmiralNeo commented on issue "Stop the GPG verification madness":
(https://github.com/bitcoin/bitcoin/issues/25395#issuecomment-1431826011)
What would be the appropriate other threads as BTC Core has stopped loading and I don't have any idea how to start it again?
<img width="812" alt="Bildschirmfoto 2023-02-15 um 19 29 00" src="https://user-images.githubusercontent.com/125405780/219120373-6cd0d7c0-a88a-4761-8555-dbe74943ef56.png">

💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431827200)
@theuni

Thank you!

> Again, I suggest a staging repo for review. It could just be your personal repo where you PR to a branch. Review happens in chunks (as you're doing now) but to that branch, that way we don't have the temporary clutter in the master repo. Then, once all the chunks have been individually merged and reviewed, it is submitted as one giant pr to master where it can be tested in full before final merge. This is essentially the [Dictator and Lieutenants Workflow](https://git
...

💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431835177)
>
> Like [that](https://github.com/hebasto/bitcoin/pull/5)?

Exactly, thank you!

So, right, I'd like to request that everyone who has been involved in reviewing (technically) the other CMake PRs to come along with me to reviewing over at @hebasto's staging repo. Since CMake is apparently so urgent, I'm sure we'll have no trouble recruiting review over there :p

Maybe it won't end up working, but I think it's worth a shot. It's at least preferable to trying to do everything 1 PR here.

💬 achow101 commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#issuecomment-1431846290)
ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1107566345)
done

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1431851194)
Updated per feedback. Thanks.
Single line removal [diff](https://github.com/bitcoin/bitcoin/compare/fa1ce3dd62be739b579373287c5501d99fcf9c17..52f4d567d69425dfd514489079db80483024a80d).

💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431855486)
> So, right, I'd like to request that everyone who has been involved in reviewing (technically) the other CMake PRs to come along with me to reviewing over at @hebasto's staging repo. Since CMake is apparently so urgent, I'm sure we'll have no trouble recruiting review over there :p

Just in case, I've made an announcement in #bitcoin-core-builds IRC channel.

💬 jonatack commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107561322)
e1910f6743d73a extra newline at EOF

Unrelated, would it make sense to avoid a `CService` copy in the most frequent case?

```diff
CService MaybeFlipIPv6toCJDNS(const CService& service)
{
+ if (service.m_net != NET_IPV6 || service.m_addr[0] != CJDNS_PREFIX || !g_reachable_nets.Contains(NET_CJDNS)) return service;
CService ret{service};
- if (ret.m_net == NET_IPV6 && ret.m_addr[0] == CJDNS_PREFIX && g_reachable_nets.Contains(NET_CJDNS)) {
- ret.m_net = NET_CJDNS;
-
...

💬 ryanofsky commented on issue "build: multiprocess link issues on fedora aarch64":
(https://github.com/bitcoin/bitcoin/issues/26943#issuecomment-1431948144)
I don't understand why this would happen only for one platform, but based on the error message maybe the following change would fix it:

```diff
--- a/depends/packages/capnp.mk
+++ b/depends/packages/capnp.mk
@@ -11,6 +11,7 @@ $(package)_config_opts := --with-external-capnp
$(package)_config_opts += CAPNP="$$(native_capnp_prefixbin)/capnp"
$(package)_config_opts += CAPNP_CXX="$$(native_capnp_prefixbin)/capnp-c++"
$(package)_config_opts_android := --disable-shared
+$(package)_cxxflags
...

💬 pinheadmz commented on issue "Exposed / Compromised (must sweep) / Used / Imported (?) flags on addresses":
(https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-1432007152)
I think this issue can be closed, 0.13.0 introduced HD wallets by default, and then recently descriptor wallets by default in 23.0.

💬 achow101 commented on issue "Exposed / Compromised (must sweep) / Used / Imported (?) flags on addresses":
(https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-1432012553)
> I think this issue can be closed, 0.13.0 introduced HD wallets by default, and then recently descriptor wallets by default in 23.0.

I don't think HD wallets sufficiently resolves this issue. We can still import things, reuse addresses, have descriptors be compromised, etc. so having these annotations would still be useful.

💬 willcl-ark commented on issue "Exposed / Compromised (must sweep) / Used / Imported (?) flags on addresses":
(https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-1432016546)
Do we still show addresses from the old SPKM(s) in the GUI after encrypting? My understanding was that they were no longer marked as `active` and so I assumed they would not show there (but I never tested it out).

💬 achow101 commented on issue "Exposed / Compromised (must sweep) / Used / Imported (?) flags on addresses":
(https://github.com/bitcoin/bitcoin/issues/3314#issuecomment-1432041881)
> Do we still show addresses from the old SPKM(s) in the GUI after encrypting?

Yes, the address book is a thing.

📝 jonatack opened a pull request: "net: remove orphaned CSubNet::SanityCheck()"
(https://github.com/bitcoin/bitcoin/pull/27106)
`CSubNet::SanityCheck()` was added in #20140, and not removed in #22570 when it became orphaned code.

Also, remove an out-of-date `snprintf` TODO that was resolved in #27036, and fix up 2 words to make the spelling linter green again.