Skip to content

WIP: abci++: Propagating vote extensions#8375

Closed
thanethomson wants to merge 14 commits intomasterfrom
sergio/8272-propagating-vote-extensions
Closed

WIP: abci++: Propagating vote extensions#8375
thanethomson wants to merge 14 commits intomasterfrom
sergio/8272-propagating-vote-extensions

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

// 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.


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 thanethomson force-pushed the sergio/8272-propagating-vote-extensions branch from 4e6a556 to 246d3c2 Compare April 19, 2022 19:09
@thanethomson
Copy link
Contributor Author

Superseded by #8433.

@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.

3 participants