Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the Past Finality Signatures #4004

Merged
merged 6 commits into from
Jun 13, 2023
Merged

Add the Past Finality Signatures #4004

merged 6 commits into from
Jun 13, 2023

Conversation

real-felix
Copy link
Contributor

Adds a new field in the block body to keep track of the past finality signatures.
Fixes #3911

@real-felix real-felix changed the base branch from dev to feat-2.0 May 31, 2023 11:43
fizyk20
fizyk20 reviewed May 31, 2023
View reviewed changes
Copy link
Collaborator

@fizyk20 fizyk20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but there is a concern about JsonBlockBody which we should resolve.

node/src/types/block.rs Show resolved Hide resolved
node/src/types/block.rs Outdated Show resolved Hide resolved
node/src/types/block.rs Outdated Show resolved Hide resolved
node/src/types/block.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Show resolved Hide resolved
Fraser999
Fraser999 reviewed Jun 1, 2023
View reviewed changes
node/src/types/block.rs Outdated Show resolved Hide resolved
node/src/types/block.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Outdated Show resolved Hide resolved
node/src/types/block/past_finality_signatures.rs Show resolved Hide resolved
Fraser999
Fraser999 approved these changes Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me except for @fizyk20's comment regarding the JsonBlockBody. I'd be fine to merge this PR as is, so long as we have a ticket to address this for 2.0.

@real-felix
Copy link
Contributor Author

JsonBlockBody::past_finality_signatures is replaced with Vec<u8> where the numbers are 1 or 0, representing the fact the corresponding validator has signed, or not.

fizyk20
fizyk20 approved these changes Jun 8, 2023
View reviewed changes
Copy link
Collaborator

@fizyk20 fizyk20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

rafal-ch
rafal-ch approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

EdHastingsCasperAssociation
EdHastingsCasperAssociation reviewed Jun 9, 2023
View reviewed changes
node/src/types/block/past_finality_signatures.rs
Debug,
Default,
)]
pub struct PastFinalitySignatures(Vec<u8>);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in standup, I would like for us to workshop better terminology. We can merge with this name for now, but by the time we ship it we will ideally have refactored it to a less vague name.

That aside, I think the payload of this tuple struct should be a map keyed by block hash or block height and a value of vec u8. This will allow the discussed scheme of permitting proposers to include signatures from earlier blocks which were missed by the proposer(s) of earlier block(s).

Again, we could merge as is for now and revisit if that is more convenient.

EdHastingsCasperAssociation
EdHastingsCasperAssociation approved these changes Jun 9, 2023
View reviewed changes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but with expectation of further iteration.

@EdHastingsCasperAssociation
Copy link
Collaborator

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 9, 2023
@casperlabs-bors-ng @real-felix
Merge #4004
b279a98 
4004: Add the Past Finality Signatures r=EdHastingsCasperLabs a=Boiethios

Adds a new field in the block body to keep track of the past finality signatures.
Fixes #3911

Co-authored-by: Félix <felix@casperlabs.io>
@casperlabs-bors-ng
Copy link
Contributor

Timed out.

@real-felix
Copy link
Contributor Author

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 10, 2023
@casperlabs-bors-ng @real-felix
Merge #4004
49ef2de 
4004: Add the Past Finality Signatures r=Boiethios a=Boiethios

Adds a new field in the block body to keep track of the past finality signatures.
Fixes #3911

Co-authored-by: Félix <felix@casperlabs.io>
@casperlabs-bors-ng
Copy link
Contributor

Timed out.

@real-felix
Copy link
Contributor Author

bors r+

casperlabs-bors-ng bot added a commit that referenced this pull request Jun 11, 2023
@casperlabs-bors-ng @real-felix
Merge #4004
12e8d62 
4004: Add the Past Finality Signatures r=Boiethios a=Boiethios

Adds a new field in the block body to keep track of the past finality signatures.
Fixes #3911

Co-authored-by: Félix <felix@casperlabs.io>
@casperlabs-bors-ng
Copy link
Contributor

Timed out.

@AlexanderLimonov
Copy link
Contributor

We're failing the NCTL upgrade test @fizyk20 @Boiethios

https://drone-auto-casper-network.casperlabs.io/casper-network/casper-node/8467/2/4

@fizyk20
Copy link
Collaborator

fizyk20 commented Jun 12, 2023

This is likely because we changed the Block struct - this would render the blocks from before the upgrade unreadable to the node. We'll have to plan how to execute an upgrade to 2.0 after we make such a change - likely a migration of storage will be involved.

@real-felix real-felix changed the base branch from feat-2.0 to feat-signature-rewards June 13, 2023 11:21
@fizyk20 fizyk20 merged commit 799d0ee into casper-network:feat-signature-rewards Jun 13, 2023
2 of 3 checks passed
@real-felix real-felix deleted the 3911-past-finality-signature branch June 14, 2023 09:56
@RitaMAllenCA RitaMAllenCA mentioned this pull request Jun 14, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderLimonov AlexanderLimonov AlexanderLimonov left review comments

@Fraser999 Fraser999 Fraser999 approved these changes

@fizyk20 fizyk20 fizyk20 approved these changes

@EdHastingsCasperAssociation EdHastingsCasperAssociation EdHastingsCasperAssociation approved these changes

@rafal-ch rafal-ch rafal-ch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce block payload field for finality signatures
6 participants
@real-felix @EdHastingsCasperAssociation @AlexanderLimonov @fizyk20 @Fraser999 @rafal-ch