Skip to content

Conversation

dknopik
Copy link
Member

@dknopik dknopik commented Jun 3, 2025

As aggregation has already been performed in the last third of the previous slot, we no longer need the selection proof for the duty at the current slot.

See also the discussion at #7016 (comment)

CC @michaelsproul

As aggregation has already been performed in the last third of the previous slot, we no longer need the selection proof for the duty at the current slot.
@dknopik
Copy link
Member Author

dknopik commented Jun 5, 2025

@chong-he
Opposed to my comment in linked PR, I decided to not also add 1 to pre_compute_slot as in prepare_for_aggregator_pre_compute we take the last_slot_of_period as upper bound. Adding 1 to that causes another off-by-one, as that slot would then belong to the next period.

@dknopik
Copy link
Member Author

dknopik commented Jun 5, 2025

I'll also try to add a test somewhere

@dknopik
Copy link
Member Author

dknopik commented Jun 24, 2025

@michaelsproul
Regarding testing this:

We already have verify_full_sync_aggregates_up_to in the simulator. Simulation is done for 16 epochs, and a sync committee period is 8 epochs, so we are testing this. Introducing an off-by-one intentionally by changing

    for slot in (start_slot..=pre_compute_slot.as_u64()).map(Slot::new) {

to

    for slot in (start_slot..pre_compute_slot.as_u64()).map(Slot::new) {

causes an error as expected: called Result::unwrap()on anErr value: "Sync aggregate at slot 17 was not full, got: 0, expected: 32"

Therefore I do not think we need another test explicitly for this.

@michaelsproul michaelsproul added the v7.1.0 Post-Electra release label Jun 24, 2025
@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Jun 24, 2025
mergify bot added a commit that referenced this pull request Jun 24, 2025
Copy link

mergify bot commented Jun 25, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #7642.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Jun 25, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Jun 25, 2025
Copy link

mergify bot commented Jun 25, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #7643.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Jun 25, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Jun 25, 2025
@mergify mergify bot merged commit a0a6b93 into sigp:unstable Jun 25, 2025
31 checks passed
@dknopik dknopik deleted the fix-sync-selection-slot branch June 25, 2025 13:29
michaelsproul added a commit that referenced this pull request Jun 29, 2025
… inbound requests (#7663)

Squashed commit of the following:

commit 0ce690c
Author: João Oliveira <hello@jxs.pt>
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 <jimmy@sigmaprime.io>
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 <jimmy@sigmaprime.io>
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 <pawandhananjay@gmail.com>
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 <107140945+dknopik@users.noreply.github.com>
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 <44791194+chong-he@users.noreply.github.com>
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 <eserilev@ucsc.edu>
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 <michael@sigmaprime.io>
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 <44791194+chong-he@users.noreply.github.com>
Date:   Mon Jun 23 16:37:49 2025 +0800

    Implement `validator_identities` Beacon API endpoint (#7462)

    * #7442
michaelsproul added a commit that referenced this pull request Jul 1, 2025
… inbound requests (#7663)

Squashed commit of the following:

commit 0ce690c
Author: João Oliveira <hello@jxs.pt>
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 <jimmy@sigmaprime.io>
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 <jimmy@sigmaprime.io>
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 <pawandhananjay@gmail.com>
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 <107140945+dknopik@users.noreply.github.com>
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 <44791194+chong-he@users.noreply.github.com>
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 <eserilev@ucsc.edu>
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 <michael@sigmaprime.io>
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 <44791194+chong-he@users.noreply.github.com>
Date:   Mon Jun 23 16:37:49 2025 +0800

    Implement `validator_identities` Beacon API endpoint (#7462)

    * #7442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v7.1.0 Post-Electra release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants