💬 achow101 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1107398449)
In e86214692d3fe7a8c835e07f7ebd47a9afb9fec9 "wallet: set keypool_size instead of access global args manager"

The `using ScriptPubKeyMan::ScriptPubKeyMan;` line above should be removed so that this new constructor is the only constructor to avoid possible programming errors where the keypool size is not passed in.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1431696802)
`7a93d7b0f9...8991ed2c6e`: address suggestions

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107408761)
Ignoring to minimize the size of the diff.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107409772)
Done.

💬 hebasto commented on pull request "guix: consolidate to glibc 2.27 for Linux builds":
(https://github.com/bitcoin/bitcoin/pull/27029#discussion_r1107418495)
Instead of removing this test entirely, maybe replace it with a symbol which was introduced glibc [2.28](https://abi-laboratory.pro/index.php?view=changelog&l=glibc&v=2.28)?

For example,
```python
with open(source, 'w', encoding="utf8") as f:
f.write('''
#define _GNU_SOURCE
#include <unistd.h>
#include <fcntl.h>

int main()
{
fcntl64(0, F_GETFD);

...

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1107422252)
Added `[[nodiscard]]` and one `AssertLockNotHeld()` where it is not immediately followed by `LOCK()`. I do not see the point in:

```cpp
AssertLockNotHeld(m); // the point of this is to crash if m is held
LOCK(m); // but this will crash if m is held, so what is the point?
```

If it is really desirable to have `AssertLockNotHeld()` before every `LOCK()`, then `AssertLockNotHeld()` should be put inside `LOCK()`.

This is another story:

```cpp
AssertLockNotHeld(m);
if (x) {
LOCK
...

💬 fanquake commented on pull request "guix: consolidate to glibc 2.27 for Linux builds":
(https://github.com/bitcoin/bitcoin/pull/27029#discussion_r1107427700)
How will we compile that in a glibc 2.27 environment?

💬 hebasto commented on pull request "guix: consolidate to glibc 2.27 for Linux builds":
(https://github.com/bitcoin/bitcoin/pull/27029#discussion_r1107444710)
> How will we compile that in a glibc 2.27 environment?

Right... :man_facepalming:

👍 hebasto approved a pull request: "guix: consolidate to glibc 2.27 for Linux builds"
(https://github.com/bitcoin/bitcoin/pull/27029)

💬 achow101 commented on pull request "Fix minor typo":
(https://github.com/bitcoin/bitcoin/pull/27102#issuecomment-1431774221)
While contributions to documentation are appreciated, trivial single line fixes that provide no clear benefits are generally discouraged and tend to be noise that distracts from and competes with review of actual substantial code changes. Please refer to our [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy) for information on what kinds of PRs are acceptable.

✅ 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