💬 fanquake commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1660543177)
Also not sure. This might remove the duplicate `AmountFromValue` from bitcoin-tx, but instead we end up with an extra wrapper function in rpc/util.h?

Generally, I think that anytime you're putting the return type of a function into it's name, something is going a bit wrong. Given in this case it's because we've already got a function called `AmountFromValue`, someone might ask why not just refactor that to return `util::Result`, instead of adding a new layer of indirection.

👍 willcl-ark approved a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1557235174)
ACK 1e65aae806

Left one more observation since last time related to the mutex locking. I'm not sure how important the Recusive Mutex-removal project is in general and whether it's worth changing though.

I would also be happy to re-review if the nits were addressed in here FWIW :)

💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1280786062)
```suggestion
AssertLockHeld(m_nodes_mutex);
```

We lock `m_nodes_mutex` here, but this is only called from inside `ForEachNode`, which itself has already locked the same mutex. It's no problem because `m-Nodes_mutex` is Recursive, but as there is a passive [effort to replace Recursive Mutexes](https://github.com/bitcoin/bitcoin/issues/19303), if you do end up touching this again perhaps you could assert the lock is held at runtime with `AssertLockHeld` and decorate header declaration w
...

💬 fanquake commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1660554671)
Going to suggest closing #28189 and just dealing with the final changes here. We can't change commit messages after the fact, so if that's a concern for some, we should correct them. I think with range-diff, and two reviewers already commited to re-ACKing, we can integrate the final changes and get this merged without issue.

💬 fanquake commented on pull request "blockstorage: Drop legacy -txindex check":
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1660561218)
cc @TheCharlatan

🤔 stickies-v reviewed a pull request: "rpc, util: deduplicate AmountFromValue() using util::Result"
(https://github.com/bitcoin/bitcoin/pull/28134#pullrequestreview-1557292966)
I think I'm approach NACK. A lot of change, and a more cluttered `rpc/util` interface just for the sake of removing this function from `bitcoin-tx`.

Isn't something like this a lot less overhead, if this is something we want to improve? https://github.com/stickies-v/bitcoin/commit/d5130411219e1f7f2791663eb22f3f4213a325ed . I'm not sure introducing the `rpc/util.h` dependency is even worth it, but that's happening in this implementation too so at least that's not worse.

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1660573780)
> Why does the previous releases task need persistent worker?

I did that to detect silent merge conflicts without using Cirrus CI resources. (A machine is running this task in a loop on all pull requests, constantly rebased on `master`). Unrelated: See https://github.com/MarcoFalke/DrahtBot/blob/main/rerun_ci/src/main.rs#L30 for the details on how to re-run.

> Is the persistent worker hosted by you? Are there any documented details about it?

Currently it is running on someone's leftover
...

💬 stickies-v commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1660574309)
Also nit: I don't understand why 709acdc359edbdc7afed15d12278cfc24220376b and 072af572b41d840a2ca53ffb3c8a1613dd4610c4 are 2 commits, that's just making review more difficult?

🚀 fanquake merged a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060)

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660581208)
Thank you for the reviews @Sjors and @MarcoFalke,

Updated 0280dc44d227ae34e8fcbb708833bfe9d66c7b5f -> e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518 ([cleaveLeveldbHeaders_0](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_0) -> [cleaveLeveldbHeaders_1](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_0..cleaveLeveldbHeaders_1))
* Addressed @MarcoFalke's [comment](https://github.c
...

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280829946)
Thank you, this is much simpler indeed.

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280830109)
I think we are in the `CDBWrapper::` scope here.

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1280831815)
:+1:

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1280838572)
I deleted the comment.
Thank you

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1660598041)
> Needs rebase, if still relevant

thanks, fixed!

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1660599083)
Rebased and switched to `Result<void>`.

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660604025)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518 🗞

<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: re-ACK e8a3a0a0d761c8544e2
...

💬 fanquake commented on pull request "doc: Clarify that -fstack-reuse=all bugs exist on all versions of GCC":
(https://github.com/bitcoin/bitcoin/pull/28105#issuecomment-1660609740)
I see the commentary on the GCC bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348#c30, that someone claims the issue still exists, but the test-case no-longer works. Should we add a note to our test case to note that it's no-longer useful post GCC 12?

🚀 fanquake merged a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166)

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660615210)
ACK fa8d99d8932df08320f1bfb4e8f1d4b4169466fd (first commit)

I have no issues with Rust, or using it in the CI; we already do in other CIs in the Bitcoin Core project. Saying that, I'm not sure if we want to introduce a 4th way to do linting in this repo (yet). One potential alternative, if we go further down the clang-tidy route, is the `<filesystem>` linter here: https://github.com/fanquake/bitcoin/commits/integrate_thecharlatan_filesystem, put together by @TheCharlatan.