💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1660491860)
concept ACK

I read the linked cirrus guide about persistent workers, everything makes sense with these changes. Two questions:
1. Is the persistent worker hosted by you? Are there any documented details about it?
2. Why does the `previous releases` task need persistent worker?

💬 MarcoFalke commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1660492118)
Needs rebase

💬 MarcoFalke commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-1660494465)
Maybe mark as draft for as long as CI is red and this is based on something else?

🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930)
> What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

Right now, this will hardly happen. The reason is `CheckForStaleTipAndEvictPeers`. The logic is as follows:

When the node doesn't update the tip for a period longer than 30 minutes (`TipMayBeStale()`), it triggers the extra outbound connection process. Which instructs the net layer to
...

💬 MarcoFalke commented on pull request "blockstorage: Drop legacy -txindex check":
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1660503311)
Thanks, fixed up the scripted-diff

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