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.

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

WIP: abci++: Propagating vote extensions #8375

Closed
wants to merge 14 commits into from

Conversation

thanethomson
Copy link
Contributor

This follows on from #8141.

@thanethomson thanethomson force-pushed the sergio/8272-propagating-vote-extensions branch from 3981cec to 4e6a556 Compare April 19, 2022 12:42
@@ -222,7 +223,7 @@ func (pool *BlockPool) PeekTwoBlocks() (first *types.Block, second *types.Block)
}

// PopRequest pops the first block at pool.height.
// It must have been validated by 'second'.Commit from PeekTwoBlocks().
// It must have been validated by 'second'.Commit from PeekTwoBlocks(), TODO: (?) and its corresponding ExtendedCommit.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be changed with the verification in any case. The verification will include vote extensions where applicable. But it is worth adding this comment here for reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm aware that however strict we want to be with the current code, it won't be "enough" (as compared with the work @jmalicevic is doing).
That's why I'd propose to leave a comment if saying something like ("TODO: the first block should also be checked with the received extended commit")

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I have WIP draft PR for the changes so we are aware of what is going on: #8419.

@@ -76,7 +76,7 @@ type Reactor struct {
stateStore sm.Store

blockExec *sm.BlockExecutor
store *store.BlockStore
store sm.BlockStore
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for my understanding, why is the internal/store replaced with the store from internal/state?

Copy link
Contributor

Choose a reason for hiding this comment

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

When I was navigating this code, I saw other places were using the blockstore interface, rather than the object directly... so I thought I'd change this for consistency... but I might be missing the bigger picture

@thanethomson
Copy link
Contributor Author

Superseded by #8433.

@sergio-mena sergio-mena added this to In progress in ABCI++ via automation May 2, 2022
@thanethomson thanethomson removed this from In progress in ABCI++ May 4, 2022
@sergio-mena sergio-mena deleted the sergio/8272-propagating-vote-extensions branch June 10, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants