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

💬 dergoegge commented on pull request "Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1494263678)
Concept ACK

💬 hebasto commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1494267607)
> Ok, so it seems we agree there is no code quality benefit.

To clarify my point, this change _does_ improve code performance when the build is configured with `--enable-debug`.

I'm not saying that this fact justifies this PR sufficiently, but it should be considered, at the very least.

> Is the motivation = CI already failing or wanting to add --enable-debug to it?

I'd avoid it as it will change code paths being parsed now (for example, the `DEBUG_LOCKCONTENTION` macro). Maybe just
...

📝 fanquake opened a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after #27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1492606272.

This change would add a depends option such that, if someone wants to build with depends, for Windows, without harde
...

💬 fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1494311710)
cc @hebasto @TheCharlatan

💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1494313712)
> This PR introduced a regression when building with depends and disabled hardening:

Followup for discussion in 27406.