-
Notifications
You must be signed in to change notification settings - Fork 977
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.
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
Confirmation Rule #3339
Open
saltiniroberto
wants to merge
171
commits into
ethereum:dev
Choose a base branch
from
saltiniroberto:confirmation_rule
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Confirmation Rule #3339
Changes from 166 commits
Commits
Show all changes
171 commits
Select commit
Hold shift + click to select a range
d137dbe
Change to filter_block_tree required for the confirmation rule
saltiniroberto e04c678
First draft implementation of the confirmation rule
saltiniroberto 0a2e654
Rename safe-head.md
saltiniroberto d51ae0c
Require to run isConfirmed only on blocks from the current epoch
saltiniroberto 3c145ec
Move get_leaf_block_roots to unused function section
saltiniroberto 124eaba
Compute remaining voting weight starting from current_slot + 1 rather…
saltiniroberto 1b3eb7c
Fix typo
saltiniroberto da75a4e
Make mypy happy
saltiniroberto e622582
Fix formula in isOneConfirmed
saltiniroberto 7cd6f41
Change ffg weight calculation and fix error in other fomula
saltiniroberto 4be510e
Account for proposer boost
saltiniroberto be8bfce
Fix minor issue
saltiniroberto e4ad646
Fix will_block_checkpoint_be_justified_by_end_of_the_current_epoch + …
saltiniroberto 9bd335c
Merge branch 'ethereum:dev' into confirmation_rule
saltiniroberto f454b99
minor updates
saltiniroberto 6f56d90
Increase style check line length for ease of development
saltiniroberto dc95b43
Use latest_message for computing get_ffg_weight_supporting_checkpoint…
saltiniroberto 89a2818
Add helper function for calculating score
saltiniroberto a68c853
Add functions to calculate score
saltiniroberto c29053b
Use underscore for all function names
saltiniroberto a6c91b4
Removed files committed by mistake
saltiniroberto a69530d
Remove other files committed by mistake
saltiniroberto 5f8e88e
Moved functions for calculating the confirmation score to a separate
saltiniroberto 3b6afe5
Added very brief explainer
saltiniroberto ad97ef9
Add back get_safe_execution_payload_hash
saltiniroberto bcc1c3f
Revert ffg support commputation to use attestations in leaf blocks
saltiniroberto 4d7b715
Doc
saltiniroberto c67cf35
Make mypy happy
saltiniroberto c3a0f7f
doctoc
saltiniroberto 2493cb3
doc
saltiniroberto d338119
Add doc and fix bug in get_max_adversary_percentage_to_ensure_block_c…
saltiniroberto 55bdbe0
Merge remote-tracking branch 'upstream/dev' into confirmation_rule
saltiniroberto 08e3abd
Removed unused files
saltiniroberto 3e29857
Remove unrequired files
saltiniroberto 3066ad4
first pass for review
adiasg d53a44a
Merge pull request #2 from saltiniroberto/confirmation_rule_adiasg
saltiniroberto 9354dc7
Small fix to the doc
saltiniroberto 1dff966
Calculate vote_weight_till_now as diff between total and future
saltiniroberto bd85b41
Fix link in bellatrix fork choice
saltiniroberto 16b26c1
review pass 2
adiasg 9ccf48b
Merge pull request #3 from saltiniroberto/confirmation_rule_adiasg
saltiniroberto b8ac374
Merge remote-tracking branch 'upstream/dev' into confirmation_rule
saltiniroberto b99e31f
update confirmation score calc
adiasg 0117eed
remove current_slot parameter
adiasg 446baeb
use simplified comm weight calc for safe block
adiasg 673b4f4
add function for computing proposer score
adiasg 38ace65
remove old code
adiasg ca5006d
use get_checkpoint_block
adiasg 9475b62
readability refactor
adiasg 668c97d
use float calculations
adiasg 6ccc528
Fix get_proposer_score
saltiniroberto 97dc8be
Fix lint error
saltiniroberto 0b2751d
Merge pull request #5 from saltiniroberto/confirmation_rule_adiasg_ro…
adiasg 49a1365
fix lint errors
adiasg 6a75d4e
update toc
adiasg 03b194e
function renaming
adiasg b843dad
wip - update conf score comments
adiasg 185338c
fix multiplier
adiasg 5c38f67
remove comments
adiasg ba7cb1f
simplify confirmation score functions
adiasg c0efc70
attach paper
adiasg 0041a00
Fix calculation bug in remaining_slots_in_current_epoch
saltiniroberto d8cbfbc
Merge pull request #6 from saltiniroberto/confirmation_rule_adiasg_ro…
adiasg 569366b
minor update
adiasg dca4c34
Merge pull request #4 from saltiniroberto/confirmation_rule_adiasg
adiasg d792899
change typing
adiasg 0d9e1a1
Update introductory notes
adiasg ee3991f
rename conflicting function
adiasg 8b3ab9c
typo correction
adiasg 79ffac1
apply review from @hwwhww and @mkalinin
adiasg 448ffac
function name case suggestion by @mkalinin
adiasg c79df70
some more suggestions from @mkalinin
adiasg 91643d9
Apply suggestions from code review
adiasg 17d528a
commit change
saltiniroberto 50e94fa
Fix small bug
saltiniroberto 988b875
Port FFG support calculation to Altair logic
saltiniroberto b8c2ba6
Limit confirmation score negative values to -1
saltiniroberto bb4a6aa
Fix https://github.com/ethereum/consensus-specs/pull/3339#discussion_…
saltiniroberto 783762a
Fix doc
saltiniroberto 956a0b7
Fix doc
saltiniroberto 28e3f64
Fix blank lines
saltiniroberto bfe43ce
Fix types in get_committee_weight_between_slots
saltiniroberto ddf8772
Move from floats to ints
saltiniroberto ce5670a
Update doc to reflect usage of unbound int rather than floats
saltiniroberto dd22c68
Moved missed float computations to int and fixed linting
saltiniroberto 9933161
Replace float with integer arithmetic for calculations in get_committ…
saltiniroberto 1269ea5
Fix https://github.com/ethereum/consensus-specs/pull/3339/files#r1230…
saltiniroberto b9919b6
Fix bug in fetching checkpoint state in get_ffg_confirmation_score an…
saltiniroberto 0530af7
Make flake8 happy
saltiniroberto 24b66c8
Use pulled up checkpoint state in get_ffg_support
saltiniroberto 616ae4c
Fix bug in get_committee_weight_between_slots
saltiniroberto ae0bf67
Revert "Fix bug in get_committee_weight_between_slots"
saltiniroberto 9bcefc1
Fix bug in get_committee_weight_between_slots
saltiniroberto 444a7e6
Fix bug in get_ffg_support
saltiniroberto b0b93f8
Fix bug in get_confirmation_score
saltiniroberto a3c463f
Fix div by 0 in get_ffg_confirmation_score
saltiniroberto d80b701
Add slashing_th par to get_confirmation_score and improve pre div-by-…
saltiniroberto 3ea5598
Allow is_confirmed for blocks from the previous epoch
saltiniroberto 3ce6a0e
Extend handling of previous epoch to get_score
saltiniroberto 8737f14
Use block state for balances and refactor get_remaining_weight
saltiniroberto 72fb786
Fix return type issue in get_committee_weight_between_slots
saltiniroberto 0e2e77a
Refactor get_ffg_confirmation_score to reduce code repetition
saltiniroberto 1fb41c4
Refactor is_ffg_confirmed to reduce code duplication
saltiniroberto 9a45487
doctoc
saltiniroberto 0d93f11
Add tests
saltiniroberto 3e4837a
Refactor tests
saltiniroberto 2aba14f
Add get_confirmation_score checks in tests
saltiniroberto a77a8b0
Make linter happy
saltiniroberto 12de731
Apply some of djrtwo’s review suggestions
saltiniroberto 2760c72
Improve calculation for edge case in get_committee_weight_between_slots
saltiniroberto 4433813
Improve get_committee_weight_between_slots (more precise calculations…
saltiniroberto 78bcb38
Remove leftovers from previous code refactoring
saltiniroberto c32b9ef
Aligned long function defs to the convention used in the rest of the …
saltiniroberto 7333dde
Simplify check on block epoch
saltiniroberto bb7fe9a
Remove duplicate line.
saltiniroberto b69dfb6
Fix doc for is_ffg_confirmed & helpers
saltiniroberto 20c101f
Rename vars in is_confirmed
saltiniroberto b1d631a
Combine code for previous and current epoch in is_ffg_confirmed and g…
saltiniroberto ad268ec
Use inline comments style rather than docstrings for comments related…
saltiniroberto e29d0ac
Fix bug in get_committee_weight_between_slots
saltiniroberto e955658
Move config par to Configuration, add constant definition and improve…
saltiniroberto 913260b
Implement early exit criteria for is_lmd_confirmed
saltiniroberto 5e05100
Add link to committee weight estimation, introduce adjustment factor …
saltiniroberto ae1ef29
Use compute_slots_since_epoch_start in get_committee_weight_between_s…
saltiniroberto 24b6db2
Fix variable names in get_committee_weight_between_slots
saltiniroberto 1340706
Use ceiling division everywhere in get_committee_weight_between_slots
saltiniroberto 49a2a30
Replace pythonic starred expression in get_leaf_block_roots with for …
saltiniroberto c41b95d
Rename var in get_ffg_support
saltiniroberto 6961150
Allow for blocks from the previous epoch in find_confirmed_block, re-…
saltiniroberto f29df8b
Fix the level of indentation of some of the headings
saltiniroberto d243ce8
Add test for get_safe_beacon_block_root
saltiniroberto c59f1fe
Temporary fork choice spec test change
saltiniroberto f4a7a4f
Fix typo
saltiniroberto 39d8b4f
Add new line to make the linter happy
saltiniroberto 33b555d
Add new line to make linter happy
saltiniroberto cfa521d
Run confirmation rule tests only from bellatrix onwards
saltiniroberto 7dfdcd7
Merge branch 'upstream/dev' into confirmation_rule
saltiniroberto 0b8eb0b
Fix possible rounding issue in get_committee_weight_between_slots
saltiniroberto 499e4f6
Improve doc on using unbounded integers
saltiniroberto a9597c4
Restore safe-block.md for now
saltiniroberto 528bbdc
Fix mistake when restoring safe-block.md in previous commit
saltiniroberto 4926681
Merge commit fork-choice-changes-for-confirmaton-rule into confirmati…
saltiniroberto e735a58
Fix error from previous merge
saltiniroberto 8c23187
Fix comments
saltiniroberto 6692612
Fix var name in get_committee_weight_between_slots
saltiniroberto cd0ad10
Rename low lever get_confirmation* functions to compute_confirmation*
saltiniroberto e365bc4
Use cell_div rather than the explicit formula
saltiniroberto d25b7ef
Rename committee_spans_full_epoch and committee_for_block_spans_full_…
saltiniroberto 9fa4fca
Change implementation of ceil_div
saltiniroberto 992dea0
Apply suggestions from @mkalinin's review
saltiniroberto 508327e
Add handling of block from epoch prior to previous in is_confirmed
saltiniroberto d6c25e1
Calculate confirmation score for blocks prior to previous epoch as well
saltiniroberto e927509
Make the linter happy
saltiniroberto 32232f6
Remove get_safe_beacon_block_root and get_safe_execution_payload_hash…
saltiniroberto 3787e96
Apply suggestions from code review
saltiniroberto 8ff01f5
Stop recursion earlier in compute_lmd_confirmation_score
saltiniroberto cacdc04
Refactor test to improve code reuse
saltiniroberto eebbe94
Merge commit dev branch into confirmation_rule
saltiniroberto 21b9d2e
Fix issue in previous merge
saltiniroberto dff96e3
Merge 'dev' into confirmation_rule
saltiniroberto c9caf62
Update to match latest paper
saltiniroberto 740be9a
Fix lint issues
saltiniroberto 688aa19
Improve doc
saltiniroberto 4ad1cc2
Fix typo
saltiniroberto be3648d
Renaming
saltiniroberto b89e93b
Fix lint issues
saltiniroberto 696e22d
Merge branch 'dev' into pr3339
hwwhww ff3daaa
resolve conflict
hwwhww e0dc09b
Fix argument sequence error
hwwhww e6d9150
Move confirmation rule specs to Bellatrix
hwwhww 687fd5c
Clean up phase0
hwwhww File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am hesitant to make it configurable with the configuration shared by all the clients. IMHO, client diversity would be better in this case. The way these parameters should be passed into runtime and also the default values can be decided by each client team independently, i think we should rather give recommendation than prescription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that each client could use a different configuration.
@adiasg @hwwhww
What do you think about this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each client has an option to override the configuration I think but usually configuration isn't changed in the clients runtime. We should check with client devs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to define the default values in this config. Sophisticated users are always able to load their own configs, while normal users should be ok using the default value from here (which should be carefully chosen - whether that's
20
or something else!).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are. A set of parameters in this and similar configs defines protocol parameters for a specific network, mainnet in this case. IMO, these parameters are not supposed to change in runtime and should not be a subject to a node operator's choice. From this perspective what we want for the confirmation rule doesn't fit a network configuration bur rather fits a client runtime parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkalinin
But compared to "Constants" and "Presets", "Configurations" is the most configurable parameter set we have in the specs. Do you suggest that we define a new category of the parameters?
To provide test vectors, we still need to set the parameter value somewhere.