💬 jonatack commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1879829758)
> I like the simplified interface, having to write something like `LogPrintLevel(BCLog::NET, BCLog::Level::Error, ...)` for each log would be annoying

The idea behind the severity level logging was to have a single macro like `Log(category, level)` with a consistent, simple interface. See https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1605951639, above.

👋 hebasto's pull request is ready for review: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875)

🤔 jonatack reviewed a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1807616432)
Light ACK a6745f8be306a0aeeb7fb070297ba503321891d0 pending the actions above

Could probably squash. IIUC closes https://github.com/bitcoin/bitcoin/pull/29123.

💬 jonatack commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1879863374)
Concept ACK

💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443869992)
Done.

💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1443870383)
Done. Also, moved this condition down to after the written coin statistic output, as it is still useful to see in the EOF-not-reached-yet case.

💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1879866925)
> Could mark as draft while CI is red?

Sorry for the extra-late reply, missed this message and the CI fail. Rebased on master and resolved the silent merge conflict (caused by the module move `test_framework.muhash` -> `test_framework.crypto.muhash` in #28374). Also fixed the Windows CI issue by closing the sqlite connections properly with explicit `con.close()` calls (see e.g. https://github.com/bitcoin/bitcoin/pull/28204). CI is green now.

>> What is the rationale for encoding as text ra
...

💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1879869514)
The CI has been fixed and this PR is ready for review now.

Friendly ping @dergoegge :)

🤔 theStack reviewed a pull request: "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1807648209)
Concept ACK

I agree with the outlined drawbacks of `struct.{un,}pack` (especially the "format string consists of characters that may be confusing and may need to be looked up in the documentation" part) and that using `int.{from,to}_bytes` should be used instead.

Small side-note unrelated to this PR:
For (de)serializing single bytes I usually prefer `bytes([val])` over `val.to_bytes(1, "little")` (and `bs[0]` over `int.from_bytes(bs, "little")`), as it feels a bit weird having to specify
...

🤔 pablomartin4btc reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1807655447)
> addressed [#27114 (comment)](https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365833858) and [#27114 (comment)](https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1440760716)

re ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d

(nit: if you have to touch 2nd and/ or 4th commits, naming could be shorter)

💬 niftynei commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1879925212)
> Assuming you mean the sequence emitted from Transaction{AddedTo,RemovedFrom}Mempool, note that this is not actually the case (they're usually off by 1 or more due to when the sequence increments happen).

Can you say more about this divergence or point me to a doc that contains more info?

I should add that one problem with the existing `getrawmempool` RPC semantics is that currently requesting the mempool_sequence *and* asking for verbose results is specifically disallowed.

I attempte
...

💬 niftynei commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1879925440)
> Also, [missing](https://github.com/bitcoin/bitcoin/pull/29016/checks?check_run_id=19389652409) test coverage for the new command:

Yes, will add test coverage when this approach gets a concept ack.

Instead, working usage of this proposed code change has been included in the original PR

💬 hebasto commented on issue "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`":
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1880029036)
According to [Wikipedia](https://en.wikipedia.org/wiki/Xcode#Xcode_15.0_-_(since_visionOS_support)_2), the Apple clang version 15.0.0 (clang-1500.0.40.1) corresponds to LLVM clang 16.0.0.

On Ubuntu 23.10:
```
$ clang++-16 -Xclang -internal-isystem/usr/local/include test.cpp
error: unknown argument: '-internal-isystem/usr/local/include'
```

However,
```
$ clang++-15 -Xclang -internal-isystem/usr/local/include test.cpp # Works.
```

📝 hebasto opened a pull request: "ci: Switch native macOS CI job to Xcode 15.0"
(https://github.com/bitcoin/bitcoin/pull/29195)
The same Xcode version is used for the release binaries.

This PR:
- addresses https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439433156
- fixes https://github.com/bitcoin/bitcoin/issues/29174

💬 hebasto commented on pull request "ci: Switch native macOS CI job to Xcode 15.0":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1880035579)
https://github.com/bitcoin/bitcoin/actions/runs/7437962301/job/20236177232:
```
checking whether C++ preprocessor accepts -Xclang -internal-isystem -Xclang /usr/local/include/... yes
```

💬 hebasto commented on issue "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`":
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1880038733)
Fixed in #29195.

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1880038943)
> If someone created a separate pull request to bump xcode in the macOS CI, that'd be great.

Please see #29195.

📝 hebasto opened a pull request: "ci: Do not set inane value for `LD_LIBRARY_PATH`"
(https://github.com/bitcoin/bitcoin/pull/29196)
For example, in the native macOS CI job, the `LD_LIBRARY_PATH=/Users/runner/work/bitcoin/bitcoin/depends/x86_64-apple-darwin/lib` makes no sense.

💬 sipa commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1880059347)
utACK fb5bfed26a564014b83ccfc96ff00b630930fc61

💬 theStack commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880061924)
Nice! Concept ACK