💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659290755)
updated PR description

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1279979090)
I think a) there is no certainty ChatGPT / LLaMa will be the most popular framework 12 / 18 months from now and I don’t think we’re going to update contributing rules everytime and b) Meta is a registered trademark of a commercial entity and I think it’s better to not give the appearance Bitcoin Core the project is supportive or supported or linked to Meta in anyway.

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1659356787)
I had the feedback from the Bitcoin Defense Legal Fund, they need more time to analyze the issue though it is thought as an important one.

I did additionally cc Andrew Chow and Fanquake as maintainers on the mail thread for open-source transparency.

I think whatever one individual political opinion on copyrights or legal risks tolerance, the fact is we have already dubious copyrights litigations affecting the project, so I think it’s reasonable to wait for risk clarification before to do a
...

💬 MarcoFalke commented on pull request "test, script: python E721 and flake8 updates":
(https://github.com/bitcoin/bitcoin/pull/28194#issuecomment-1659613454)
lgtm ACK bee2d57a655645dbfaf0242e85c5af034023a2fb

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659618783)
Approach NACK, (but Concept ACK). Leaving a NACK only to have DrahtBot create an anchor to this comment, to avoid GitHub from hiding it as well.

I don't think you've seen https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387 and GitHub is already hiding it by default.

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279295208)
```suggestion
// Ensure sensitive relay connections only send the above message types. Others are not needed and may degrade privacy.
```

(nit), to avoid having to touch this line again in the future, if something changes.

💬 glozow commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1280249089)
I think I know what you mean, but imo transactions being stuck is a little bit of a jump from feerate underestimation. Maybe "Serving stale fee estimates may lead to overpaying in fees or not meeting minimum feerate requirements in the worst case." Or just delete this comment, as the code is self-explanatory and there is already a comment above `MAX_FILE_AGE`.

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