💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1443852237)
> @jonatack do you have an opinion?

Am catching up with merged pulls and discussions; this one is high on my list when I've caught up.

💬 GregTonoski commented on issue "[Bug]: Blockspace price shouldn't be higher for a simple transaction (price discrimination against simple txs)":
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879825003)
> Yes it takes more bytes to spend a transaction than to split a transaction because input consumption includes signatures (witness data) and outputs typically contain only P2SH which is compact. So the discount balances those input consumption bytes and output creation bytes so there is no longer a financial incentive to create dust. To be sure if you run out of coins your wallet will use change, but until then it will keep splitting coins. Say you have 100 1BTC lumps in your wallet for privacy
...

💬 jonatack commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1879827022)
Will review the updates since my initial ACK https://github.com/bitcoin/bitcoin/pull/25390#pullrequestreview-1390434676.

💬 GregTonoski commented on issue "[Bug]: Blockspace price shouldn't be higher for a simple transaction (price discrimination against simple txs)":
(https://github.com/bitcoin/bitcoin/issues/29146#issuecomment-1879828846)
https://delvingbitcoin.org/t/bug-spammers-get-bitcoin-blockspace-at-discounted-price-lets-fix-it/327/4?u=gregtonoski

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