💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280201239)
029ac57bf6a987f50c31bb8c80e77989fc53d85a: nit: missing clang-format?

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280255522)
nit in 9fd6debc9f50955507d1540310ae4928b561bea0: This is assumed to never be null and heavily used. What do you think about an inline private helper:


```cpp
auto& DBContext() { return *Assert(m_db_context); }

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280202266)
029ac57bf6a987f50c31bb8c80e77989fc53d85a: nit: I don't think you need to specify `CDBIterator::` to `make_unique` when you are already in the `CDBIterator::` scope?

Also could use `{}` for the constructor, instead of `()`?

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