Replace HashSet with BitVector for permanent attestation subscriptions

[VolodymyrBg]

Issue Addressed

This PR addresses issue #XXX: Optimizing memory usage and performance of the subnet service.

Proposed Changes

• Replaces HashSet<Subnet> with BitVector for storing permanent attestation subscriptions
• Updates related methods to work with bit operations instead of HashSet operations
• Modifies the subscription checks to handle the new BitVector implementation
• Updates test methods to work properly with the new data structure

Additional Info

This change improves both memory efficiency and performance of the subnet service. BitVector is a more space-efficient representation for tracking subnet subscriptions, which is particularly important for nodes with many validators or high network activity. The implementation follows a pattern already used elsewhere in the codebase for subnet bitfields, specifically in ENR attestation and sync committee bitfields, creating a more consistent approach to subnet tracking throughout the codebase.

[CLAassistant]

All committers have signed the CLA.

[dapplion]

Hey @VolodymyrBg ! While this may reduce memory, I think the reduction is negligible. LH average memory is on the order of GBs and there are at most 64 subnets

[VolodymyrBg]

Thanks for the PR! I'm generally in agreement with @dapplion here but I also see that this was addressing a TODO so we should give it some consideration and at the very least remove the TODO if we don't think these changes are worth it.

For now I've just left a comment about the usage of expect

I've made some corrections, but if this change is not necessary I can close it.

[chong-he]

Could you please sign the CLA? Thanks

I see that the PR is currently targeting the stable branch, can you change it to unstable?

[mergify[bot]]

This pull request has merge conflicts. Could you please resolve them @VolodymyrBg? 🙏

[VolodymyrBg]

I've gone ahead and set the target branch to unstable for you. However, now your PR includes some commits from stable which we don't actually want in unstable. Can you do an interactive rebase (git rebase -i unstable) and remove any commits that you didn't author? Thanks

Thanks, i'll try

[VolodymyrBg]

I've gone ahead and set the target branch to unstable for you. However, now your PR includes some commits from stable which we don't actually want in unstable. Can you do an interactive rebase (git rebase -i unstable) and remove any commits that you didn't author? Thanks

Done

[dapplion]

I vote to remove the TODO unless someone else justifies otherwise

@AgeManning Blame points to you as author https://github.com/sigp/lighthouse/commit/08e8b92e5032044f253a7357aaf0a7c74f9f8b80#diff-e3499b786075371d4d8f0407ea9795dce66efca341265c55b1a225aa6d9ce09fR92

[AgeManning]

Oh yeah. I wrote that todo as I was implementing this change and had considered a bitvector for initial implementation but later decided it wasn't worth the effort (as I'd have to implement the dynamic bitvector).

I believe @jking-aus has made a dynamic bit vector and we probably should use that to remove the EthSpec dep.

I'll be able to properly review this next week, but I'm fine for going either way with this.

[AgeManning]

@mergifyio queue

[mergify[bot]]

queue

🛑 The pull request has been synchronized by a user

[AgeManning]

Looks like you need to solve clippy lints and run cargo fmt. To solve clippy lints, run make lint from the lighthouse directory and correct the errors.

[mergify[bot]]

This pull request has been removed from the queue for the following reason: pull request manually updated.

The pull request #7317 has been manually updated.

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

[VolodymyrBg]

@AgeManning Fixed clippy

[VolodymyrBg]

@AgeManning

[jimmygchen]

I vote to remove the TODO unless someone else justifies otherwise

@AgeManning Blame points to you as author 08e8b92#diff-e3499b786075371d4d8f0407ea9795dce66efca341265c55b1a225aa6d9ce09fR92

+1 I'm also not sure if it's worth the change, changing HashSet<Subnet> to a BitVector with only 64 elements doesn't seem to give us much. Not strongly opposed to it though.

[VolodymyrBg]

I vote to remove the TODO unless someone else justifies otherwise @AgeManning Blame points to you as author 08e8b92#diff-e3499b786075371d4d8f0407ea9795dce66efca341265c55b1a225aa6d9ce09fR92

+1 I'm also not sure if it's worth the change, changing HashSet<Subnet> to a BitVector with only 64 elements doesn't seem to give us much. Not strongly opposed to it though.

So, what to do? Should I make corrections to the code, or is it useless?

[AgeManning]

If we don't want to add the complexity (others vote to avoid it), then I think this PR is still useful for fixing the waker and maybe removing the original comment.

[mergify[bot]]

Hi @VolodymyrBg, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.