💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280242366)
6005165242cf090589934133f01f9dbd496612f1: I think you can just remove all of those classes/inheritance/constructors by just using two lambdas?


```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 94883ec669..71ec3b5f91 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -300,11 +300,11 @@ std::vector<unsigned char> CDBWrapper::CreateObfuscateKey() const
return ret;
}

-bool CDBWrapper::ReadImpl(CDBWrapper::ReaderBase& reader) const
+bool CDBWrapper::ReadIm
...

💬 Sjors commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1659794915)
With #24008 landed this would a great time for a rebase (I tried but it's non-trivial).

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1659813384)
> Can you link to 10 passing tests in your fork? Otherwise we may run into intermittent issues.

1. https://github.com/hebasto/bitcoin/actions/runs/5708399487/attempts/1
2. https://github.com/hebasto/bitcoin/actions/runs/5708399487/attempts/2
3. https://github.com/hebasto/bitcoin/actions/runs/5708399487/attempts/3
4. https://github.com/hebasto/bitcoin/actions/runs/5708399487/attempts/4
5. https://github.com/hebasto/bitcoin/actions/runs/5708399487/attempts/5
6. https://github.com/hebasto/b
...

💬 TheCharlatan commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1659814327)
Post-merge ACK a733dd79e29068ad1e0532ac42a45188a040a7b9

Very happy with the separation of concerns between blockstorage and validation.

🚀 fanquake merged a pull request: "test: Drop 22.x node from TxindexCompatibilityTest"
(https://github.com/bitcoin/bitcoin/pull/28070)

💬 MarcoFalke commented on pull request "test: Drop 22.x node from TxindexCompatibilityTest":
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1659843691)
Looks like this was merged, so I'll submit the scripted diff in the follow-up

🚀 fanquake merged a pull request: "doc: cleanup release process doc"
(https://github.com/bitcoin/bitcoin/pull/28003)

🚀 fanquake merged a pull request: "test: Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/28131)

🚀 fanquake merged a pull request: "test: python E721 and flake8 updates"
(https://github.com/bitcoin/bitcoin/pull/28194)

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