💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1280251267)
It's related to https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277639934

💬 Sjors commented on pull request "ParseHDKeypath: support h as hardened marker":
(https://github.com/bitcoin/bitcoin/pull/28192#issuecomment-1659783573)
We could always relax the constraint if someone has a good use case for not being consistent. For now this is method is only foreseen to be used with `getxpub` where I can't think of an even remotely sane reason. More likely someone will do this by accident and end up confused why their descriptor checksum doesn't match.

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1659784274)
Thanks, makes sense. I initially wondered about the 15 commits and +100 LOC, but having more commits for this template-heavy code makes it easier to review, and I found a way to reduce the number of LOC. Just a style nit though, feel free to ignore.

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280179325)
unrelated nit in 4735590b9f3bdfc248086e0e20c9d56fa6156e1c: Add missing `}; // namespace dbwrapper_private` (via clang-format) to clarify the new function is not in the namespace?

👍 MarcoFalke approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556273554)
review ACK 0280dc44d227ae34e8fcbb708833bfe9d66c7b5f 🔨

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 0280dc44d227
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280188384)
74b5c3dac581b7597668a6a0cc55678d2af296da: Any reason to remove the docs from the header?

💬 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"?