💬 fanquake commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1659899169)
I think some of the changes here are fine, but seem to be done somewhat verbosely.

There are 3 commits (5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0, df488563b280c63f5d74d5ac0fcb1a2cad489d55, 5316ae5dd8d90623f9bb883bb253fa6463ee4d34) that independently change/refactor `GetLocal()`. Any reason they can't be combined, and avoid touching the same line of code 3 times?

Speaking generally, not sure about the addition of `[nodiscard]`. It's not clear that is something that other project contributors
...

💬 willcl-ark commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1659921791)
Lightly tested ACK fa633aa690

Sidenote: reading the discussions on performance I decided to try and modify the Xor function to use AVX2 SIMD (mainly so I could learn more about it myself) and managed to get something working, but couldn't get it to run faster than the current implementation in this pull.

```log
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 9.71
...

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1659932085)
[ @willcl-ark You could try to inspect the binary after compilation to see how the compiler optimized it (and what difference there is to your version) ]

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280354671)
> Ok. You are setting one in the line above: `name: macOS 13 native, x86_64 [no depends, sqlite only, gui]`
>
> But if that is not possible to use, that is fine.

Well, it is still possible to get `github.job` property that has value `macos-native` from https://github.com/bitcoin/bitcoin/blob/e9b72185205812df51082b42fb8c25cbf53cbc03/.github/workflows/ci.yml#L14-L15

> Though, it would be good to explain why both `key` and `restore-key` are set and why `github.run_id` is used?

This use
...

💬 fanquake commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1659939766)
I mostly agree with @ryanofsky.

The reality is that going forward it'll be essentially impossible to avoid contributions that may include output from AI/LLMs, just because (in almost all cases) it'll be impossible to tell, unless the author makes it apparent.

We certainly don't want to end up in some situation where contributors are trying to "guess" or point out these types of contributions, or end up with reversion PRs (incorrectly) trying to remove certain content.

If we end up with
...

💬 MarcoFalke commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1659940762)
> It's not clear that is something that other project contributors agree with, i.e: discussion in this thread: https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254980474.

I can see the desire to enforce it everywhere, but then it should be done via clang-tidy or a clang-tidy plugin. Ideally without modifying the source code at all.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280365390)
Mind linking to the doc in this line then? Otherwise all reviewers and future readers will have to wade through the docs themselves?

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280373834)
> Well, it is still possible to get github.job property that has value macos-native from

`"macos-native"` as key seems fine (for ccache), no? (Any way is fine, though)

👍 Sjors approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556486530)
utACK 0280dc44d227ae34e8fcbb708833bfe9d66c7b5f modulo 6005165242cf090589934133f01f9dbd496612f1.

I also checked that all the intermediate commits compile and pass the tests (on Ubuntu 23.04).

I'm not familiar with the LevelBD code, but the refactor makes sense to me.

I'll look at 6005165242cf090589934133f01f9dbd496612f1 later since it might get simplified based on @MarcoFalke's suggestion.

💬 Sjors commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280311080)
74b5c3dac581b7597668a6a0cc55678d2af296da: `m_impl_batch` (since we don't use `p` for pointers) or `m_impl` (the pattern used in e.g. `AddrManImpl`)?

💬 Sjors commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280383910)
ca6f83d88cb241786676bfb341fc27f6467d39ee: this description, although not amazing, went missing

💬 fanquake commented on pull request "Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1659989600)
Needs a rebase, if still relevant, and the [question above](https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1539543097) also needs addressing.

👍 vasild approved a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1556652326)
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

I am also ok to add new functions in the same commit in which they are going to be used.

📝 MarcoFalke opened a pull request: " blockstorage: Drop legacy -txindex check "
(https://github.com/bitcoin/bitcoin/pull/28195)
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.

Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)

💬 MarcoFalke commented on pull request "test: Drop 22.x node from TxindexCompatibilityTest":
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1660034722)
Follow-up in https://github.com/bitcoin/bitcoin/pull/28195

💬 vasild commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1660036151)
_offtopic `[[nodiscard]]`_

There are 3 classes of functions wrt `[[nodiscard]]`:

1. where ignoring the result is harmful, it probably means a bug in the program, for example [write(2)](https://man7.org/linux/man-pages/man2/write.2.html)
2. where ignoring the result is ok and the code is equivalent without the call, aka it does not make sense to call the function, its call should be removed because it clutters the source code and makes the program slower, for example [strlen(3)](https://ma
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660036495)
> I'd be happy to work with a reopened #22242

I guess that pull is mostly orthogonal (shouldn't (silently) conflict with this one). Rebased and reopened in https://github.com/bitcoin/bitcoin/pull/28195

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660128648)
@MarcoFalke, I am not sure I fully understand in [your comment](https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387). Can you elaborate on the CPFP example?

In general, wrt the privacy of the transaction originator, do you think that the "sensitive relay" mechanism from this PR could be worse than the current "send to all" from `master` in some cases? Or do you think that it is better than `master` but it can be done even better?

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660192852)
> do you think that the "sensitive relay" mechanism from this PR could be worse

Yes, my understanding is that this pull strictly decreases privacy when the underlying wallet makes use of transactions chains (whether or not for intentional or accidental CPFP shouldn't matter), or transaction replacements, or mutually exclusive transactions ("conflicts", "double-spends" or insufficient rbf-fee), or if the transaction recipient is the attacker and can create transactions spending the "sensitive
...

👍 dergoegge approved a pull request: "refactor: Remove unused MessageStartChars parameters from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/28191#pullrequestreview-1556884717)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073