Make BEEFY payload extensible by seunlanlege · Pull Request #10307 · paritytech/substrate

seunlanlege · 2021-11-18T16:31:57Z

closes paritytech/grandpa-bridge-gadget#198

client/beefy/src/lib.rs

primitives/beefy/src/commitment.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

tomusdrw

Thanks for the fixes, left a few additional suggestions.

tomusdrw · 2021-11-26T16:14:33Z

primitives/beefy/src/commitment.rs

+	/// Returns a raw payload under given `id`. If the `BeefyPayloadId` is not found in the payload
+	/// `None` is returned.
+	pub fn get_raw(&self, id: &BeefyPayloadId) -> Option<&Vec<u8>> {
+		self.0.iter().find_map(


since this is sorted you can do binary_search, however with such small number of elements it wouldn't really matter.

primitives/beefy/src/commitment.rs

tomusdrw · 2021-11-26T16:19:09Z

client/beefy/src/worker.rs

+			let mut payload = Payload::default();
+			payload.push(known_payload_ids::MMR_ROOT_ID, mmr_root);


nit: we could have Payload::new to simplify this and avoid marking payload as mut.

like

let payload = Payload::new().push(known_payload_ids::MMR_ROOT_ID, mmr_root);

technically that requires modifying the push method to take self and return it so we can chain multiple calls to push

Yeah, or just Payload::new(known_payload_ids::MMR_ROOT_ID, mmr_root); - that additionally ensure that no empty payload can be created.

tomusdrw · 2021-11-26T16:20:23Z

client/beefy/src/worker.rs

 		self.gossip_validator.note_round(round.1);

-		let vote_added = self.rounds.add_vote(round, vote);
+		let vote_added = self.rounds.add_vote(round.clone(), vote);


I think it would be better to change add_vote to clone internally only when the vote is actually going to be added.

tomusdrw · 2021-11-26T16:22:04Z

primitives/beefy/src/commitment.rs

+	/// Push a value that implements [`codec::Encode`] with a given id
+	/// to the back of the payload vec. This method will internally sort the payload vec
+	/// after every push.
+	pub fn push<T: Encode>(&mut self, id: BeefyPayloadId, value: T) {


I was thinking about that kind of API as well when making a proposal, but the thing is that Vec<u8> implements Encode as well, so I think it might actually be error prone to have something like that (i.e. we might end up with double-encoded vectors).

I'd prefer to just have add_raw method.

primitives/beefy/src/commitment.rs

tomusdrw · 2021-11-26T16:23:14Z

primitives/beefy/src/commitment.rs

+	/// after every push.
+	pub fn push<T: Encode>(&mut self, id: BeefyPayloadId, value: T) {
+		self.0.push((id, value.encode()));
+		self.0.sort_by(|(id_1, _), (id_2, _)| {


Since this is guaranteed to be sorted you can actually use binary_search to find the right position instead of sorting the entire vector every time.
However given that we don't expect many values there performance wise it probably does not matter.

Using binary_search though would allow you to easily detect duplicates (which we should do), so I'd suggest changing that.

primitives/beefy/src/commitment.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

tomusdrw

Thank you! One last tiny change request :)

client/beefy/src/gossip.rs

primitives/beefy/src/commitment.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

tomusdrw · 2021-12-01T11:45:27Z

bot merge

This reverts commit 8bf9836.

* make BEEFY payload extensible * cargo fmt * cargo fmt * remove generic payload param in beefy-primitives * cargo fmt * Apply suggestions from code review Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * adds Paylaod Type * remove hex * fix tests * Apply suggestions from code review Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * use binary_search_by to sort * Payload::new() * fix tests * Apply suggestions from code review Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * fix tests * cargo fmt * fix get_decoded * fix test Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

make BEEFY payload extensible

846f92c

seunlanlege requested a review from adoerr as a code owner November 18, 2021 16:31

seunlanlege changed the title ~~Make BEEFY payload extensible closes paritytech/grandpa-bridge-gadget#198~~ Make BEEFY payload extensible Nov 18, 2021

seunlanlege added 2 commits November 18, 2021 17:33

cargo fmt

1ea70a3

cargo fmt

d8a1553

adoerr requested a review from tomusdrw November 23, 2021 05:14

seunlanlege added 3 commits November 25, 2021 11:35

remove generic payload param in beefy-primitives

d0d1d2e

Merge branch 'master' of git://github.com/paritytech/substrate

440cefd

cargo fmt

6ec8040

tomusdrw reviewed Nov 25, 2021

View reviewed changes

tomusdrw removed the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Nov 25, 2021

seunlanlege and others added 4 commits November 26, 2021 12:13

Apply suggestions from code review

bfb118d

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

adds Paylaod Type

06b4148

remove hex

6aa66b9

fix tests

bf9733f

tomusdrw reviewed Nov 26, 2021

View reviewed changes

seunlanlege and others added 5 commits November 30, 2021 09:44

Apply suggestions from code review

883c82d

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

use binary_search_by to sort

ca5ccb4

Merge branch 'master' of github.com:seunlanlege/substrate

d21aa67

Payload::new()

ae69ed0

fix tests

2fa426c

tomusdrw approved these changes Nov 30, 2021

View reviewed changes

client/beefy/src/gossip.rs Outdated Show resolved Hide resolved

primitives/beefy/src/commitment.rs Outdated Show resolved Hide resolved

seunlanlege and others added 4 commits November 30, 2021 17:07

Apply suggestions from code review

382f636

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

fix tests

399a336

Merge branch 'master' of github.com:seunlanlege/substrate

fc5d3b5

cargo fmt

dfb2182

adoerr approved these changes Nov 30, 2021

View reviewed changes

seunlanlege added 2 commits November 30, 2021 18:08

fix get_decoded

5d76251

fix test

6ec0a62

paritytech-processbot bot merged commit 8bf9836 into paritytech:master Dec 1, 2021

tomusdrw mentioned this pull request Dec 1, 2021

Change commitment payload to make it possible to easily add new items in the future. paritytech/grandpa-bridge-gadget#198

Closed

niklasad1 added a commit that referenced this pull request Dec 2, 2021

Revert "Make BEEFY payload extensible (#10307)"

c17aee7

This reverts commit 8bf9836.

		let mut payload = Payload::default();
		payload.push(known_payload_ids::MMR_ROOT_ID, mmr_root);

Conversation

seunlanlege commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw Nov 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomusdrw Nov 26, 2021

Choose a reason for hiding this comment

Uh oh!

seunlanlege Nov 27, 2021

Choose a reason for hiding this comment

Uh oh!

tomusdrw Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

tomusdrw Nov 26, 2021

Choose a reason for hiding this comment

Uh oh!

tomusdrw Nov 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomusdrw Nov 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomusdrw commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seunlanlege commented Nov 18, 2021 •

edited

Loading