💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280780213)
Ah I didn't see the last message.

> I think it would make more sense to change info_for_relay?

sounds good to me

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660512698)
Reposting my reply to this identical message on the mailing list:

On Mon, Jul 31, 2023 at 03:13:19AM -0700, Daniel Lipshitz wrote:
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC
...

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660520660)
On Sun, Jul 30, 2023 at 02:18:25PM -0700, Matt Corallo wrote:
> > This is helpful for second layer protocols, including Lightning.
>
> I'm not aware of any second layer protocols that are improved by having full-rbf more broadly available.

@ariard provided an example above.

See also: https://petertodd.org/2023/fullrbf-multiparty-protocols

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