💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765524349)
even better

✅ mzumsande closed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538)

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-2359152545)
Not working on this currently, I might open a new PR at a later time, or up for grabs if someone else wants to.

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765529514)
If we're allowing it, let's make sure there's overage for it in the fuzzer

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765537433)
ok, seems clear to me now, not sure why I was so confused.

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2359202120)
looked at `git range-diff master 9f67d6dcdcf7e9c75a93ad78f8902537599f4f6d 4476915bc76b48eff9b5e67fd6bd7647cc12793f`

LGTM, though I haven't re-validated the serializer, trusting the tests of tests. I want to think more about the fuzz targets to see if there are any other easy pickups in coverage.

📝 LuizWT opened a pull request: "Optimize: convert trusted keys list to a set for better performance"
(https://github.com/bitcoin/bitcoin/pull/30925)
# Motivation

This pull request optimizes the handling of trusted keys by converting lists to sets. This change improves performance for membership checks, making them faster.
Changes Made

Changed trusted keys storage from lists to sets for faster access.

# Tests

No new tests were added, as this is a refactor that does not change the business logic.

💬 theuni commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1765609266)
Arguably the issue here is that we have debug symbols for secp at all.

💬 laanwj commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#issuecomment-2359296620)
Have you benchmarked the performance difference?

💬 laanwj commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1765643226)
Have you even tested this? You can't subscript index into a set, and it makes no sense.

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765576120)
AddDependency doesn't exist anymore

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765664723)
if not erroneous, I think a bit stronger
```Suggestion
Assume((m_used.None() && pos_range == 0) || m_used.Last() == pos_range - 1);
```

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765657306)
is this not still true?

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765667807)
Haven't looked close enough to the new format, but pushing another element to `reordering` after hitting `SetType::Size` already seems to make the DepGraph construction later odd? Couldn't this make the mapping argument too large?

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765572322)
I think this gives additional coverage since no deps added in a `AddDependencies` call is allowed
```Suggestion
const auto parents_mask = provider.ConsumeIntegralInRange<uint64_t>(0, (uint64_t{1} << num_tx) - 1);
```

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765598717)
Could use some documentation on how to use `mapping` and `pos_range`, with holes it's not immediately obvious.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2359409218)
> 1. Child`MempoolRejectedTx`, put in orphanage
>
> 2. Child `MempoolRejectedTx` for other reason, now in reject filter

On first glance that sounds impossible, `MempoolRejectedTx` should have erased it from orphanage? Will look into the error

🤔 mzumsande reviewed a pull request: "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2313807588)
> Whereas calling a rescan on the (missing) background blocks seems like a bug that should be avoided at the call site, no?

Well, the approach seems to be similar with pruning:
`importdescriptors` tries to scan as many blocks as possible (no stopping after a failure due to missing block data), and then returns a failure / warns the user that the result may be incomplete. It could probably have been implentend differently, checking if we are missing blocks due to pruning and refuse to import
...

💬 mzumsande commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1765732245)
I think this should only be possible to trigger with the rescan, the various RPCs are always be called with a block (usually the tip) that is guaranteed to have `m_chain_tx_count` set.

💬 SanAndreyas commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1765732956)
This is not even an equivalent expression. When you convert a list to a set, the order of the lines is lost. Therefore, accessing the first element with [0] could give you any of the lines randomly, not necessarily the first line from the original file. In this case you needed index 0 ( root ).