💬 fanquake commented on pull request "rpc, util: avoid string copies in ParseHash/ParseHex functions":
(https://github.com/bitcoin/bitcoin/pull/28172#issuecomment-1659860688)
Also unsure. Can you better-explain the changes here/respond to the review comment?

💬 fanquake commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1659866165)
> and helps with using only what is needed, which may reduce build size

What do you mean by "build size"?

💬 fanquake commented on pull request "test: update tool_wallet coverage for unexpected writes to wallet":
(https://github.com/bitcoin/bitcoin/pull/28116#issuecomment-1659869447)
> Will rebase once that PR is merged.

Mark as draft for now then, if based on another PR?

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280315850)
> any reason to not just use the task name, like before?

GHA's job (an analogue of Cirrus's task) [context](https://docs.github.com/en/actions/learn-github-actions/contexts#job-context) has no "name"-like property.

💬 hebasto commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1659882376)
Rebased on top of the merged #28188.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280323073)
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. Though, it would be good to explain why both `key` and `restore-key` are set and why `github.run_id` is used?

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