Replace HashSet with BitVector for permanent attestation subscriptions #7317

VolodymyrBg · 2025-04-11T15:24:08Z

Issue Addressed

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

Proposed Changes

Replaces HashSet 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 · 2025-04-11T15:24:17Z

All committers have signed the CLA.

dapplion · 2025-04-11T21:28:49Z

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

macladson

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

beacon_node/network/src/subnet_service/mod.rs

VolodymyrBg · 2025-04-12T14:12:52Z

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 · 2025-04-14T05:06:19Z

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 · 2025-04-14T10:53:56Z

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

macladson

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

VolodymyrBg · 2025-04-14T11:33:10Z

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 · 2025-04-14T14:07:27Z

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 · 2025-04-23T15:04:22Z

I vote to remove the TODO unless someone else justifies otherwise

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

AgeManning · 2025-04-24T22:16:35Z

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

I think this is fine.

This is something that @jking-aus might need to update when the dynamic bitfield is ready. I'll chase him up, maybe we could include it in this PR.

AgeManning · 2025-05-02T01:58:07Z

beacon_node/network/src/subnet_service/mod.rs

-                permanent_attestation_subscriptions
-                    .insert(Subnet::Attestation(SubnetId::from(index)));
+                let index_usize = index as usize;
+                // Instead of using expect, handle any potential error and log it


Nice. We probably don't need this comment tho.

beacon_node/network/src/subnet_service/mod.rs

Co-authored-by: Age Manning <[email protected]>

AgeManning · 2025-05-12T01:35:59Z

@Mergifyio queue

mergify · 2025-05-12T01:36:17Z

queue

🛑 The pull request has been synchronized by a user

AgeManning · 2025-05-12T01:40:27Z

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 · 2025-05-12T01:41:17Z

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 · 2025-05-12T17:56:57Z

@AgeManning Fixed clippy

VolodymyrBg · 2025-06-11T15:23:55Z

@AgeManning

jimmygchen · 2025-06-27T05:11:52Z

beacon_node/network/src/subnet_service/mod.rs

@@ -767,3 +822,6 @@ impl PartialEq for SubnetServiceMessage {
        }
    }
 }
+
+// Реализуем Unpin для SubnetService, чтобы можно было использовать get_mut() на Pin<&mut Self>


Could you write this comment in English please?

jimmygchen · 2025-06-27T05:24:03Z

beacon_node/network/src/subnet_service/mod.rs

-            if waker.will_wake(cx.waker()) {
-                self.waker = Some(cx.waker().clone());
+        if let Some(waker) = &this.waker {
+            if !waker.will_wake(cx.waker()) {


Nice catch!

jimmygchen · 2025-06-27T05:56:49Z

beacon_node/network/src/subnet_service/mod.rs

-                permanent_attestation_subscriptions
-                    .insert(Subnet::Attestation(SubnetId::from(index)));
+                let index_usize = index as usize;
+                if let Err(e) = permanent_attestation_subscriptions.set(index_usize, true) {


This relies on spec.attestation_subnet_count (a runtime config) matching EthSpec::SubnetBitFieldLength (a compile time constant), which is a hard requirement, we could just fail the startup if this happens?

jimmygchen · 2025-06-27T13:54:46Z

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 · 2025-06-27T18:35:07Z

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?

@VolodymyrBg

This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else. The old logic updated the waker when it already matched the context, and did nothing when it was stale: ```rust if waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` This is the wrong way around. We only want to update the waker if it doesn't match the current context: ```rust if !waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups.

AgeManning · 2025-06-29T23:29:40Z

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.

@VolodymyrBg

… inbound requests (#7663) Squashed commit of the following: commit 0ce690c Author: João Oliveira <[email protected]> Date: Fri Jun 27 22:50:11 2025 +0100 Error from RPC `send_response` when request doesn't exist on the active inbound requests. And handle the error from the main service to check if it may be a data race or a critical bug commit 522e00f Author: Jimmy Chen <[email protected]> Date: Sat Jun 28 05:01:46 2025 +1000 Fix incorrect `waker` update condition (#7656) This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else. The old logic updated the waker when it already matched the context, and did nothing when it was stale: ```rust if waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` This is the wrong way around. We only want to update the waker if it doesn't match the current context: ```rust if !waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups. commit 83cad25 Author: Jimmy Chen <[email protected]> Date: Sat Jun 28 04:21:17 2025 +1000 Fix Rust 1.88 clippy errors & execution engine tests (#7657) Fix Rust 1.88 clippy errors. commit 9b1f3ed Author: Pawan Dhananjay <[email protected]> Date: Thu Jun 26 17:26:38 2025 -0700 Add gossip check (#7652) N/A Add an additional gossip condition. commit a0a6b93 Author: Daniel Knopik <[email protected]> Date: Wed Jun 25 08:22:24 2025 +0200 Do not compute sync selection proofs for the sync duty at the current slot (#7551) commit 8e3c5d1 Author: chonghe <[email protected]> Date: Wed Jun 25 13:33:17 2025 +0800 Rust 1.89 compiler lint fix (#7644) Fix lints for Rust 1.89 beta compiler commit 56b2d4b Author: Eitan Seri-Levi <[email protected]> Date: Tue Jun 24 09:29:10 2025 +0300 Remove instrumenting log level (#7636) #7155 Theres some additional places we set instrumenting log levels that wasn't covered in #7620 commit fd643c3 Author: Michael Sproul <[email protected]> Date: Mon Jun 23 23:11:46 2025 +1000 Un-ignore EF test for v1.6.0-alpha.1 (#7632) Closes: - #7547 Run the test that was previously ignored when we were between spec versions. commit cef04ee Author: chonghe <[email protected]> Date: Mon Jun 23 16:37:49 2025 +0800 Implement `validator_identities` Beacon API endpoint (#7462) * #7442

@VolodymyrBg

… inbound requests (#7663) Squashed commit of the following: commit 0ce690c Author: João Oliveira <[email protected]> Date: Fri Jun 27 22:50:11 2025 +0100 Error from RPC `send_response` when request doesn't exist on the active inbound requests. And handle the error from the main service to check if it may be a data race or a critical bug commit 522e00f Author: Jimmy Chen <[email protected]> Date: Sat Jun 28 05:01:46 2025 +1000 Fix incorrect `waker` update condition (#7656) This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else. The old logic updated the waker when it already matched the context, and did nothing when it was stale: ```rust if waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` This is the wrong way around. We only want to update the waker if it doesn't match the current context: ```rust if !waker.will_wake(cx.waker()) { self.waker = Some(cx.waker().clone()); } ``` I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups. commit 83cad25 Author: Jimmy Chen <[email protected]> Date: Sat Jun 28 04:21:17 2025 +1000 Fix Rust 1.88 clippy errors & execution engine tests (#7657) Fix Rust 1.88 clippy errors. commit 9b1f3ed Author: Pawan Dhananjay <[email protected]> Date: Thu Jun 26 17:26:38 2025 -0700 Add gossip check (#7652) N/A Add an additional gossip condition. commit a0a6b93 Author: Daniel Knopik <[email protected]> Date: Wed Jun 25 08:22:24 2025 +0200 Do not compute sync selection proofs for the sync duty at the current slot (#7551) commit 8e3c5d1 Author: chonghe <[email protected]> Date: Wed Jun 25 13:33:17 2025 +0800 Rust 1.89 compiler lint fix (#7644) Fix lints for Rust 1.89 beta compiler commit 56b2d4b Author: Eitan Seri-Levi <[email protected]> Date: Tue Jun 24 09:29:10 2025 +0300 Remove instrumenting log level (#7636) #7155 Theres some additional places we set instrumenting log levels that wasn't covered in #7620 commit fd643c3 Author: Michael Sproul <[email protected]> Date: Mon Jun 23 23:11:46 2025 +1000 Un-ignore EF test for v1.6.0-alpha.1 (#7632) Closes: - #7547 Run the test that was previously ignored when we were between spec versions. commit cef04ee Author: chonghe <[email protected]> Date: Mon Jun 23 16:37:49 2025 +0800 Implement `validator_identities` Beacon API endpoint (#7462) * #7442

mergify · 2025-07-29T23:31:14Z

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.

macladson reviewed Apr 12, 2025

View reviewed changes

beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved

beacon_node/network/src/subnet_service/mod.rs Outdated Show resolved Hide resolved

chong-he added the ready-for-review The code is ready for review label Apr 14, 2025

chong-he added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 14, 2025

macladson changed the base branch from stable to unstable April 14, 2025 10:53

macladson requested a review from jxs as a code owner April 14, 2025 10:53

macladson reviewed Apr 14, 2025

View reviewed changes

VolodymyrBg added 2 commits April 14, 2025 17:03

Replace HashSet with BitVector for permanent attestation subscriptions

da0c82a

Refactor BitVector operations to use proper error handling

44f5910

VolodymyrBg force-pushed the bg branch from 10b363b to 44f5910 Compare April 14, 2025 14:07

VolodymyrBg requested a review from macladson April 14, 2025 14:07

chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 15, 2025

AgeManning approved these changes May 2, 2025

View reviewed changes

VolodymyrBg and others added 2 commits May 2, 2025 15:38

Update beacon_node/network/src/subnet_service/mod.rs

0b10dfe

Co-authored-by: Age Manning <[email protected]>

Update beacon_node/network/src/subnet_service/mod.rs

6b3e0eb

Co-authored-by: Age Manning <[email protected]>

VolodymyrBg requested a review from AgeManning May 2, 2025 12:39

AgeManning approved these changes May 6, 2025

View reviewed changes

Merge branch 'unstable' into bg

ef8b5a7

mergify bot added a commit that referenced this pull request May 12, 2025

Merge of #7317

40402d6

mergify bot mentioned this pull request May 12, 2025

merge queue: embarking unstable (5933901) and #7317 together #7439

Closed

6 tasks

Merge branch 'unstable' into bg

2803d7f

fix clippy

533f016

VolodymyrBg requested a review from AgeManning May 12, 2025 17:57

jimmygchen reviewed Jun 27, 2025

View reviewed changes

jimmygchen mentioned this pull request Jun 27, 2025

Fix incorrect waker update condition #7656

Merged

jimmygchen reviewed Jun 27, 2025

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jun 27, 2025

mergify bot closed this Jul 29, 2025

mergify bot added the stale Stale PRs that have been inactive and is now outdated label Jul 29, 2025

Replace HashSet with BitVector for permanent attestation subscriptions #7317

Replace HashSet with BitVector for permanent attestation subscriptions #7317

Uh oh!

Conversation

VolodymyrBg commented Apr 11, 2025

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

CLAassistant commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dapplion commented Apr 11, 2025

Uh oh!

macladson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

VolodymyrBg commented Apr 12, 2025

Uh oh!

chong-he commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Apr 14, 2025

Uh oh!

macladson left a comment

Choose a reason for hiding this comment

Uh oh!

VolodymyrBg commented Apr 14, 2025

Uh oh!

VolodymyrBg commented Apr 14, 2025

Uh oh!

dapplion commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AgeManning commented Apr 24, 2025

Uh oh!

AgeManning left a comment

Choose a reason for hiding this comment

Uh oh!

AgeManning May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AgeManning commented May 12, 2025

Uh oh!

mergify bot commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been synchronized by a user

Uh oh!

AgeManning commented May 12, 2025

Uh oh!

mergify bot commented May 12, 2025

Uh oh!

VolodymyrBg commented May 12, 2025

Uh oh!

VolodymyrBg commented Jun 11, 2025

Uh oh!

jimmygchen Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

jimmygchen commented Jun 27, 2025

Uh oh!

VolodymyrBg commented Jun 27, 2025

Uh oh!

AgeManning commented Jun 29, 2025

Uh oh!

mergify bot commented Jul 29, 2025

Uh oh!

Uh oh!

CLAassistant commented Apr 11, 2025 •

edited

Loading

chong-he commented Apr 14, 2025 •

edited

Loading

dapplion commented Apr 23, 2025 •

edited

Loading

mergify bot commented May 12, 2025 •

edited

Loading