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

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1494228089)
@hernanmarino I have libevent pinned to the version with the matching signature function. Let me know if you want anything else changed in this PR.

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

The "tidy" and "TSan" CI tasks have been fixed.

💬 fanquake commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1494258971)
Note that even though the CI here is "green", [the tidy task](https://cirrus-ci.com/task/6643322689683456?logs=ci#L5204) has failed:
```bash
In file included from common/url.cpp:5:
In file included from ./common/url.h:8:
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/string:40:
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/char_traits.h:39:
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/
...

👍 TheCharlatan approved a pull request: "validation: Move warningcache to ChainstateManager and rename to m_warningcache"
(https://github.com/bitcoin/bitcoin/pull/27357)
ACK 552684976b6df34ce563458f73812e6e494e3b0e

📝 MarcoFalke opened a pull request: "Use steady clock instead of system clock to measure durations"
(https://github.com/bitcoin/bitcoin/pull/27405)
`GetTimeMillis` has multiple issues:

* It doesn't denote the underlying clock type
* It isn't type-safe
* It is used incorrectly in places that should use a steady clock

Fix all issues here.