[Merged by Bors] - Disallow attesting to optimistic head #3140

paulhauner · 2022-04-04T06:17:37Z

Issue Addressed

NA

Proposed Changes

Disallow the production of attestations and retrieval of unaggregated attestations when they reference an optimistic head. Add tests to this end.

I also moved BeaconChain::produce_unaggregated_attestation_for_block to the BeaconChainHarness. It was only being used during tests, so it's nice to stop pretending it's production code. I also needed something that could produce attestations to optimistic blocks in order to simulate scenarios where the justified checkpoint is determined invalid (if no one would attest to an optimistic block, we could never justify it and then flip it to invalid).

Additional Info

~~Blocked on [Merged by Bors] - Ensure VALID response from fcU updates protoarray #3126~~

realbigsean

Just had one comment, looking good though!

realbigsean · 2022-04-06T12:56:44Z

beacon_node/beacon_chain/src/beacon_chain.rs

+            .get_block_execution_status(&beacon_block_root)
+            .map_or(true, |execution_status| execution_status.is_not_verified())
+        {
+            None


I'm sort of thinking we should handle errors here like in produce_unaggregated_attestation, otherwise the HTTP API will return "missing aggregate" which would be a bit misleading

Great point, I've added that. I also jigged around with the naming too. All the meaningful changes are in bc8f220.

paulhauner · 2022-04-07T07:25:34Z

All comments addressed, thanks @realbigsean. I will merge after an approval (or change request) :)

realbigsean · 2022-04-07T16:06:46Z

consensus/proto_array/src/proto_array_fork_choice.rs

+    /// Returns `true` if the block:
+    ///
+    /// - Has execution enabled, AND
+    /// - Hash a valid payload


Suggested change

/// - Hash a valid payload

/// - Has a valid payload

realbigsean · 2022-04-07T16:26:44Z

consensus/fork_choice/src/fork_choice.rs

@@ -127,6 +127,17 @@ pub enum PayloadVerificationStatus {
    Irrelevant,
 }

+impl PayloadVerificationStatus {
+    /// Returns `true` if the payload was optimistically imported.
+    pub fn is_optimistic(&self) -> bool {


I'm wondering if PayloadVerificationStatus::NotVerified should be renamed to PayloadVerificationStatus::Optimistic as well to make this mapping very clear. I could understand if we want to keep the concept of "optimism" constrained to fork choice though. What are your thoughts?

This method can also be used in block_verification.rs L1168 by the way

Great suggestions, will do both.

realbigsean · 2022-04-07T16:30:57Z

beacon_node/http_api/src/lib.rs

+                        .map_err(|_e| {
+                            warp_utils::reject::custom_bad_request(
+                                "unable to fetch aggregate".to_string(),
+                            )
+                        })?


This error's swallowed

Suggested change

.map_err(|_e| {

warp_utils::reject::custom_bad_request(

"unable to fetch aggregate".to_string(),

)

})?

.map_err(|e| {

warp_utils::reject::custom_bad_request(format!(

"unable to fetch aggregate: {e:?}"

))

})?

Nice catch, thank you!

paulhauner · 2022-04-08T02:30:57Z

Thanks for the comments, apologies for the sloppy mistakes!

I've addressed all comments and fixed the merge conflict :)

realbigsean

Awesome! LGTM

paulhauner · 2022-04-12T03:00:12Z

I'm blocking this on the v2.2.1 release (#3149)

paulhauner · 2022-04-13T03:54:25Z

bors r+

## Issue Addressed NA ## Proposed Changes Disallow the production of attestations and retrieval of unaggregated attestations when they reference an optimistic head. Add tests to this end. I also moved `BeaconChain::produce_unaggregated_attestation_for_block` to the `BeaconChainHarness`. It was only being used during tests, so it's nice to stop pretending it's production code. I also needed something that could produce attestations to optimistic blocks in order to simulate scenarios where the justified checkpoint is determined invalid (if no one would attest to an optimistic block, we could never justify it and then flip it to invalid). ## Additional Info - ~~Blocked on #3126~~

bors · 2022-04-13T05:45:35Z

Pull request successfully merged into unstable.

Build succeeded:

## Issue Addressed NA ## Proposed Changes Disallow the production of attestations and retrieval of unaggregated attestations when they reference an optimistic head. Add tests to this end. I also moved `BeaconChain::produce_unaggregated_attestation_for_block` to the `BeaconChainHarness`. It was only being used during tests, so it's nice to stop pretending it's production code. I also needed something that could produce attestations to optimistic blocks in order to simulate scenarios where the justified checkpoint is determined invalid (if no one would attest to an optimistic block, we could never justify it and then flip it to invalid). ## Additional Info - ~~Blocked on sigp#3126~~

paulhauner added blocked work-in-progress PR is a work-in-progress bellatrix Required to support the Bellatrix Upgrade labels Apr 4, 2022

paulhauner force-pushed the opt-attn-production branch from 6b4e441 to 8be59d6 Compare April 5, 2022 21:06

paulhauner added 3 commits April 6, 2022 09:26

Disallow attn production for optimistic heads

b87dc81

Move function into test harness

60777d2

Tidy

0c714e9

paulhauner removed the blocked label Apr 5, 2022

paulhauner force-pushed the opt-attn-production branch from 8be59d6 to 0c714e9 Compare April 5, 2022 23:27

Remove unused import

46b5f26

paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 6, 2022

paulhauner marked this pull request as ready for review April 6, 2022 06:31

realbigsean reviewed Apr 6, 2022

View reviewed changes

Return Result from agg fns, rename

bc8f220

paulhauner added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Apr 7, 2022

Add comment

55e3a68

paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 7, 2022

realbigsean requested changes Apr 7, 2022

View reviewed changes

realbigsean 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 7, 2022

paulhauner added 5 commits April 8, 2022 12:12

Fix typo

fea76f9

Rename NotVerified to Optimistic

f9ddb30

Ensure error is not swallowed

4c7fedf

Use is_optimistic fn

f592aa3

Merge branch 'unstable' into opt-attn-production

bbca67f

paulhauner mentioned this pull request Apr 8, 2022

Restrict sync committee messages to optimistic blocks #3151

Closed

macladson mentioned this pull request Apr 8, 2022

[Merged by Bors] - Add execution_optimistic flag to HTTP responses #3070

Closed

22 tasks

realbigsean approved these changes Apr 8, 2022

View reviewed changes

realbigsean added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 8, 2022

paulhauner added the blocked label Apr 12, 2022

michaelsproul removed the blocked label Apr 13, 2022

bors bot changed the title ~~Disallow attesting to optimistic head~~ [Merged by Bors] - Disallow attesting to optimistic head Apr 13, 2022

bors bot closed this Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Disallow attesting to optimistic head #3140

[Merged by Bors] - Disallow attesting to optimistic head #3140

paulhauner commented Apr 4, 2022 •

edited

Loading

realbigsean left a comment

realbigsean Apr 6, 2022

paulhauner Apr 7, 2022

paulhauner commented Apr 7, 2022

realbigsean Apr 7, 2022

realbigsean Apr 7, 2022 •

edited

Loading

paulhauner Apr 8, 2022

realbigsean Apr 7, 2022

paulhauner Apr 8, 2022

paulhauner commented Apr 8, 2022

realbigsean left a comment

paulhauner commented Apr 12, 2022

paulhauner commented Apr 13, 2022

bors bot commented Apr 13, 2022

[Merged by Bors] - Disallow attesting to optimistic head #3140

[Merged by Bors] - Disallow attesting to optimistic head #3140

Conversation

paulhauner commented Apr 4, 2022 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean Apr 6, 2022

Choose a reason for hiding this comment

paulhauner Apr 7, 2022

Choose a reason for hiding this comment

paulhauner commented Apr 7, 2022

realbigsean Apr 7, 2022

Choose a reason for hiding this comment

realbigsean Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

paulhauner Apr 8, 2022

Choose a reason for hiding this comment

realbigsean Apr 7, 2022

Choose a reason for hiding this comment

paulhauner Apr 8, 2022

Choose a reason for hiding this comment

paulhauner commented Apr 8, 2022

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner commented Apr 12, 2022

paulhauner commented Apr 13, 2022

bors bot commented Apr 13, 2022

paulhauner commented Apr 4, 2022 •

edited

Loading

realbigsean Apr 7, 2022 •

edited

Loading