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

Return blocks on same chain in BeaconBlocksByRange #1005

Merged
merged 3 commits into from
Jun 25, 2020
Merged

Return blocks on same chain in BeaconBlocksByRange #1005

merged 3 commits into from
Jun 25, 2020

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Jun 12, 2020

@twoeths twoeths requested a review from a team June 12, 2020 02:39
mpetrunic
mpetrunic requested changes Jun 15, 2020
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

You should probably also change in this PR chain.getBlockAtSlot to return Array of blocks and propagate it into database. Currently if we get and process block thats on fork it will override last block linked to that slot.

@twoeths twoeths force-pushed the tuyen/strict-block-range branch from 4daa3dd to 6f560c2 Compare June 16, 2020 04:43
mpetrunic
mpetrunic requested changes Jun 16, 2020
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

I think there was misuunderstanding.
We should change how we store slot -> root mapping. Currently is 1:1 and it should be 1:n aka 1 slot can have many block roots attached. That also means that db.blocks.getBlockAtSlot should be db.blocks.getBlocksAtSlot. slot -> root mapping is 1:1 only in finalized blocks (block archive)

packages/lodestar/src/chain/interface.ts Outdated Show resolved Hide resolved
@twoeths
Copy link
Contributor Author

twoeths commented Jun 16, 2020

@mpetrunic I see what you mean now. Can you have a look at https://github.com/ethereum/eth2.0-specs/pull/1818/files

My understanding was we have 1-1 mapping for slot-block after that PR.

@mpetrunic
Copy link
Member

@mpetrunic I see what you mean now. Can you have a look at https://github.com/ethereum/eth2.0-specs/pull/1818/files

My understanding was we have 1-1 mapping for slot-block after that PR.

I think this just prevents multiple blocks per slot in single head chain. In unfinalized chain we can have parallel chains (forks) where different block proposers are proposing different blocks for same slot we need to store multiple blocks per slot until we finalize this (decide on one fork to be correct).

@twoeths
Copy link
Contributor Author

twoeths commented Jun 16, 2020

I think this just prevents multiple blocks per slot in single head chain. In unfinalized chain we can have parallel chains (forks) where different block proposers are proposing different blocks for same slot we need to store multiple blocks per slot until we finalize this (decide on one fork to be correct).

Thanks, clear to me now. With the latest implementation, I don't touch getBlockAtSlot so I'm not implementing getBlocksAtSlot in this PR, will open a separate issue for that.

Just one clarification, we don't have db.blocks.getBlockAtSlot (maybe we used to have that in the past, I'm not sure). In the meantime we get blockRoot from forkchoice and get block based on blockRoot.

@twoeths twoeths force-pushed the tuyen/strict-block-range branch 2 times, most recently from 5e76343 to 1ed8b25 Compare June 16, 2020 22:43
@twoeths twoeths requested review from a team and mpetrunic June 16, 2020 22:56
mpetrunic
mpetrunic approved these changes Jun 17, 2020
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

lgtm, although solving multiple fork blocks for same slot was probably more important than avoiding it 😞

@twoeths twoeths requested a review from a team June 18, 2020 03:25
@wemeetagain
Copy link
Member

merge conflict, looks ready to merge otherwise

@twoeths twoeths force-pushed the tuyen/strict-block-range branch from 1ed8b25 to 5939740 Compare June 25, 2020 04:54
mpetrunic
mpetrunic reviewed Jun 25, 2020
View reviewed changes
packages/lodestar/test/utils/mocks/chain/chain.ts
if (!slots) {
return [];
}
return await Promise.all(slots.map(this.getBlockAtSlot));
Copy link
Member

Choose a reason for hiding this comment

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

We could rename getBlockAtSlot to getCannonicalBlockAtSlot and keet this method getBlocksAtSlot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could rename getBlockAtSlot to getCannonicalBlockAtSlot

this is good to me

and keet this method getBlocksAtSlot

as this method only gets unfinalized blocks, should we keep it getUnfinalizedBlocksAtSlots to avoid any confusion? Or any better name?

Copy link
Member

Choose a reason for hiding this comment

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

as this method only gets unfinalized blocks, should we keep it getUnfinalizedBlocksAtSlots to avoid any confusion? Or any better name?

oh scratch that, thought this method is on forkchoice.

@wemeetagain wemeetagain merged commit d490965 into 0.12.x Jun 25, 2020
@wemeetagain wemeetagain deleted the tuyen/strict-block-range branch June 25, 2020 20:44
@twoeths twoeths mentioned this pull request Aug 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpetrunic mpetrunic mpetrunic approved these changes

@wemeetagain wemeetagain wemeetagain 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.
3 participants
@twoeths @mpetrunic @wemeetagain