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

✅ fanquake closed an issue: "p2p_getaddr_caching.py failure in TSan CI"
(https://github.com/bitcoin/bitcoin/issues/28133)

💬 MarcoFalke 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-1660617362)
It may be better to replace it with one that still reproduces. Though, there is an array of at least 5 of those bugs, presumably all due to the same underlying GCC bug. Instead of constantly updating the test case to the latest one that still reproduces, maybe just remove the outdated test case?