Free the fork choice #666
Free the fork choice #666
Conversation
ralexstokes
force-pushed
the
free-the-fork-choice
branch
from
ab3da5a
to
aadf358
Compare
|
@hwwhww given that you mentioned you would make a pass at this -- another option here is to keep EDIT: scratch this -- we still have the issue of tying the fork choice to the block import w/ this route... readers should ignore this comment, the problem that was here has been resolved in this PR :)
def lmd_ghost_fork_choice_scoring(attestation_pool):
return lambda block, chaindb: run_lmd_ghost(block, chaindb, attestation_pool)
# something using a `ChainDB`:
scoring = lmd_ghost_fork_choice_scoring(get_attestation_pool())
new_blocks, old_blocks = chaindb.persist_block(block, block.__class__, scoring)
|
ralexstokes
force-pushed
the
free-the-fork-choice
branch
from
23373a6
to
8242407
Compare
| return chaindb | ||
|
|
||
|
|
||
| @pytest.fixture(params=[0, 10, 999]) | ||
| @pytest.fixture(params=[1, 10, 999]) | ||
| def block(request, sample_beacon_block_params): |
There was a problem hiding this comment.
to not conflict w/ genesis block
|
ok! this is a little messy as it tracked my thinking about the best way to get towards a more flexible fork choice :) after I rebase, it should be ready to merge pending review. the tests all pass locally but circleCI is down at the moment. |
In particular, it does not make sense to persist a block without running the fork choice. We will need some method to update the canonical head when running the fork choice outside of importing a block, but let's prefer to handle that in a downstream PR.
ralexstokes
force-pushed
the
free-the-fork-choice
branch
from
8242407
to
40538dd
Compare
ralexstokes
force-pushed
the
free-the-fork-choice
branch
6 times, most recently
from
7e09ea0
to
e0b8f3b
Compare
ralexstokes
force-pushed
the
free-the-fork-choice
branch
from
e0b8f3b
to
f67a99d
Compare
ralexstokes
force-pushed
the
free-the-fork-choice
branch
from
f67a99d
to
7cb4903
Compare
|
just to update, this is officially ready for review now :) |
|
|
||
| # methods | ||
| @staticmethod | ||
| def create_block_from_parent(parent_block: BaseBeaconBlock, | ||
| block_params: FromBlockParams) -> BaseBeaconBlock: | ||
| return create_serenity_block_from_parent(parent_block, block_params) | ||
|
|
||
| def get_fork_choice_scoring(self) -> ForkChoiceScoring: | ||
| return higher_slot_scoring |
There was a problem hiding this comment.
How would the lmd_ghost looks like in this method?
Does the method produce new functions for every slot and block?
My imagination:
def get_fork_choice_scoring(self) -> ForkChoiceScoring:
def scoring(block):
return lmd_ghost_scoring(db=self.chaindb.db, attestation_pool=somewhere.attestation_pool, state=self.state, start_block=somewhere.start_block, target_block=block, config=self.config)
return scoringThere was a problem hiding this comment.
yeah more or less! modulo those parameters changing, e.g. i don't think the StateMachine has a reference directly to the chain_db and similar for the other dependencies...
i'll think a bit about the best way to keep a unified function interface while still giving us flexibility...
there is also a concern about how often this closure is created -- it would be possible to memoize this function, esp if the attestation pool is a reference to the mutable thing, not just a description of attestations -- this route generally goes against our preference for immutability though so i'd be inclined to find something more, well, immutable :)
There was a problem hiding this comment.
@ChihChengLiang latest thinking here:
like i say in the PR, i'm still chewing on a clean way to further modularize the fork choice so it is not so tightly bound to a particular StateMachine.
There was a problem hiding this comment.
Sorry for the late review!
Rationale for putting the fork choice scoring in the StateMachine and using it in the Chain class
if we change the fork choice (at some slot), does that imply a new state machine?
My two cents:
- Agreed that it’s not ideal to embed the fork choice scoring in
BeaconChain! - However, regarding the efficiency, at this point I’m not sure if the fork choice scoring upgrade would be necessary for the future. (Comparing to eth1 fork choice rule didn’t change much).
- If we are certain that the fork choice rule upgrade is exactly as the
StateMachine(forks) upgrade boundary, we can make sure that in eachbatchis using the same fork choice rule scoring function (based on the TBD wire protocol), so that we may be able to initialize lessstate_machines during syncing.
- If we are certain that the fork choice rule upgrade is exactly as the
state_machine.get_fork_choice_scoring()function refactoring: instead of returning aCallable, it seems similar to how eth1eth/vm/forks/overrides the static methods. For example, thecompute_difficultyinFrontierand inHomesteadVM. I think we can utilize this pattern inSerenityStateMachine. Defer to the snake charmers on which pattern is more appropriate in this case. :)
|
|
||
| from eth2.beacon.types.blocks import BaseBeaconBlock | ||
|
|
||
| ForkChoiceScoring = Callable[[BaseBeaconBlock], int] |
There was a problem hiding this comment.
What do you think about naming it ForkChoiceScoringFn?
There was a problem hiding this comment.
yeah appending Fn seems ok
There was a problem hiding this comment.
my thinking was that earlier in this process I wasn't sure this abstraction would just be a Callable.... the ForkChoice could evolve so that it is more convenient to have more state, in which case we could just go ahead w/ a class and ForkChoiceScoring would become an abstract class (for the interface)...
but i think we can get away w/ just a Callable so let's do that for now
@hwwhww yeah i went down this route before I landed on the current thing but decided to do this mainly because I ran into issues w/ my thinking was to just do the simple thing to keep moving :) the one caveat is that w/ LMD GHOST there is basically a "setup" step where we need to fetch the latest justified state and it seems much more ergonomic as an instance method... i'll refer you here: https://github.com/ethereum/trinity/pull/685/files#diff-26594f899e3bcc74e868bbab3824d8f9R46 |
|
unless anyone has further comment or concern, i'll go ahead and merge this soon... i'll address #666 (comment) in #685 which builds off of this PR |
Right, I recalled it was really weird: ethereum/py-evm#1362 (comment) :'(
Sounds good to me! |
EDIT: update description to reflect current state of PR, may be helpful to start w/
Summary of changesbelowWhat was wrong?
Big part of implementing LMG GHOST fork choice.
Pulling this out of #532 as it is substantial enough to stand on its own.
In Eth1, the fork scoring function is just a function of the block header. This fact makes it easy to calculate the score as we import new headers, adjusting the canonical head along the way.
In Eth2, the fork scoring function is more complex (a function of the block, the state, and off-chain attestations); moreover, we want to run the fork choice in more situations than just importing a new valid block.
The current design of the fork choice calculation does not readily facilitate these tasks.
How was it fixed?
Summary of changes:
persist_blockto allow arbitrary scoringset_scoreAPI (not technically required for this PR but will need for future fork choice work. easier to include here than patch up the git history... forgive me for the lack of precision :) )To address these limitations, we allow the caller of
persist_blockto provide their own scoring logic.The DB class now exposes methods to persist the fork choice but does not actively compute it (at least via the new APIpersist_block_without_scoring). The particular fork choice lives to a particular variation of theStateMachine. For example, theSerenityStateMachinewill have thelmd_ghost_fork_scoringrule after it lands in a subsequent PR. For now, they just use the existinghigher_slot_scoringrule.Rationale for putting the fork choice scoring in the StateMachine and using it in the Chain class
The Chain class is what ultimately orchestrates validating incoming data, persisting valid data, running the fork choice (based on the current state machine's scoring rule) and persisting the results of this fork choice (i.e. writing the new block's score and updating the canonical head if necessary).
This configuration is more flexible, exposing a direct path to inject the AttestationPool necessary for LMD GHOST, and avoids the situation where the DB knows about every other object in the system (single responsibility and all that).
I have some concerns mainly around the new API(s). It might be dangerous to expose something likeset_scorefor anyone to call andupdate_canonical_head_if_neededfeels a little clunky still... I would suggest we merge this in and as we gain confidence w/ the new stuff, we can look at deprecating the old stuff (namely,persist_blockand its callers).Other comments
We could make the argument to pull the actual process of selecting which fork to follow (given a "score chain") but this seems low value given that we can map any fork choice to "follow the monotonically increasing score" by warping the fork choice scoring function).
There is another concern around "overloading" the state machine w/ the fork choice -- it seems like the best place to put it for now but readers should note that the state machine is no longer just a transformation from block to block. To justify this change, let's answer the question: if we change the fork choice (at some slot), does that imply a new state machine?
To-Do
I'll add more tests but given there are some design questions up in the air, I'll get this up so you can make a pass at reviewing it if you want...
Cute Animal Picture