Skip to content

Propose every nth block as finality candidate instead of all the blocks#93

Merged
gaudenzkessler merged 14 commits intodevelopfrom
gk/propose_nth_block
Oct 27, 2022
Merged

Propose every nth block as finality candidate instead of all the blocks#93
gaudenzkessler merged 14 commits intodevelopfrom
gk/propose_nth_block

Conversation

@gaudenzkessler
Copy link
Contributor

@gaudenzkessler gaudenzkessler commented Sep 21, 2022

belongs to #1003

@gaudenzkessler gaudenzkessler force-pushed the gk/propose_nth_block branch 3 times, most recently from d63802e to 12ec509 Compare September 21, 2022 14:12
@gaudenzkessler gaudenzkessler marked this pull request as ready for review October 21, 2022 10:44
Propose every nth block as finality candidate instead of all the blocks
@gaudenzkessler gaudenzkessler changed the base branch from master to develop October 21, 2022 11:57
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

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

To how I understood integritee-network/worker#1003 the following should be done in the parentchain:

  • store the next_finalization_candidate_block_number or whatever you call it, it's a u64, so that's fine, somewhere as a state.
  • Check if the block_number sent with this xt, matches the previously stored next_finalization_candidate_block_number. If it does, finalize that block hash.

This PR however adds a diff and checks if that diff plus the last finalized (?) block number match the current block number. If so, the that block is finalized. So the finalization of the block depends on the block_diff sent by the very same xt, not the one sent previously. Or did I misunderstand something?

We can take that offline if you'd like to, I think I finally understood integritee-network/worker#1003

@gaudenzkessler
Copy link
Contributor Author

ok I can turn it around.

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Looks good in general. I have some minor remarks though. :)

@OverOrion
Copy link
Contributor

LGTM, thank you, I especially liked that the function finalize_blocks_from_queue has been deleted. 🚀

Copy link
Contributor

@coax1d coax1d left a comment

Choose a reason for hiding this comment

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

LGTM

@gaudenzkessler gaudenzkessler requested review from OverOrion and clangenb and removed request for haerdib October 26, 2022 14:35
Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding the asserts in the tests! Only one thing remains. :)

@gaudenzkessler gaudenzkessler merged commit 3590bee into develop Oct 27, 2022
@gaudenzkessler gaudenzkessler deleted the gk/propose_nth_block branch October 27, 2022 13:10
Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback! I found one last thing. :)

shard7,
1,
block_number,
block_number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please call the finalization candidate next_finalization_candidate? :)

Comment on lines +116 to 120
ensure!(
block_number == finalization_candidate_block_number,
<Error<T>>::ReceivedUnexpectedSidechainBlock
);

Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of your test made me realize one more test that we need to do:

ensure!(
	next_finalization_candidate_block_number > finalization_candidate_block_number
	<Error<T>>::InvalidNextFinalizationCandidateBlockNumber
);

OverOrion added a commit that referenced this pull request Nov 11, 2022
OverOrion pushed a commit that referenced this pull request Nov 11, 2022
…ks (#93)

Propose every nth block as finality candidate instead of all the blocks


Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>
OverOrion added a commit that referenced this pull request Nov 11, 2022
clangenb pushed a commit that referenced this pull request Nov 15, 2022
* Propose every nth block as finality candidate instead of all the blocks (#93)

Propose every nth block as finality candidate instead of all the blocks


Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>

* Add validity check to finalization (#111)

* Add validity check to finalization

* adjust test

* adjust benchmark test

Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>

* fixup! Propose every nth block as finality candidate instead of all the blocks (#93)

Co-authored-by: gaudenzkessler <92718752+gaudenzkessler@users.noreply.github.com>
Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>
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.

6 participants