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

💬 glozow commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1493961578)
> The CI was previously failing on the functional tests due to that issue.

How is it possible for that functional test to fail due to using iters vs references to iters?
Wasn't the issue with #27316 not waiting for sync, i.e. addressed by #27318?
Does the problem still exist?
Still trying to understand the motivation for this PR.

💬 fanquake commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1493963338)
The functional test failure was sporadic, and unrelated to the changes proposed here.

💬 xJCPMSx commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1493971510)
Top

💬 dergoegge commented on pull request "refactor: Continue moving application data from CNode to Peer":
(https://github.com/bitcoin/bitcoin/pull/26621#issuecomment-1493984109)
Rebased

🤔 dergoegge requested changes to a pull request: "reduce cs_main scope, guard block index 'nFile' under a local mutex"
(https://github.com/bitcoin/bitcoin/pull/27006)
Concept ACK on reducing `cs_main` scope.

Approach NACK for adding new globals. It is not worth going with a worse design to "rush in" an improvement for a feature that may or may not happen (#26966). I would prefer to go with a more sustainable approach from the get go, otherwise this will need to be refactored in the long run.

I don't see why the mutex could not be a member of `BlockManager`. Like you say in https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1481222148, giving `Bl
...

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1494093330)
re-ACK 45926671dbf9e4919bf6aaee90e0d90f89830ce5

Nit: consider moving the typo fixes to the first commit (or just squash both).

💬 glozow commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1155770419)
nit: it would be nice to have a `bin` example as well
```suggestion
./contrib/verifybinaries/verify.py pub bitcoin-core-23.0-rc1
./contrib/verifybinaries/verify.py bin ~/Downloads/SHA256SUMS ~/Downloads/bitcoin-24.0.1-x86_64-linux-gnu.tar.gz
```

💬 glozow commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1155785561)
nit: This seems like it can be deleted, since 2 is an allowed return code and missing "the Bitcoin Core binary release signing key" doesn't apply

```suggestion
```

👍 stickies-v approved a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)
ACK f842ed9a40c0db656b86f85e84dd4978865cc0a0

Easier to read, less duplication.

💬 stickies-v commented on pull request "test: refactor: replace unnecessary `BytesIO` uses":
(https://github.com/bitcoin/bitcoin/pull/27389#discussion_r1155810011)
nit: I suppose technically this is behaviour change, since we're going from default endianness to little endianness? But I suspect previously the test would have failed on big endian platforms, and being explicit is a strict improvement. Just flagging in case it does lead to weird behaviour somewhere.

📝 fanquake opened a pull request: "ci: use clang-16 in tidy task"
(https://github.com/bitcoin/bitcoin/pull/27404)
Follow up to https://github.com/bitcoin/bitcoin/pull/27311#issuecomment-1481020371, as IWYU now has a [clang_16 branch](https://github.com/include-what-you-use/include-what-you-use/tree/clang_16).

Drafted while tracking down an issue with `stddef.h` inclusion..

💬 hebasto commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1494131422)
Concept ACK.

💬 Sjors commented on issue "Unable to create PSBT for legacy watchonly wallets in the GUI":
(https://github.com/bitcoin/bitcoin/issues/26687#issuecomment-1494205436)
I was able to reproduce this issue and indeed #26699 fixes it.