Skip to content
This repository has been archived by the owner on Nov 11, 2022. It is now read-only.

Optimize encoded-size of SignedCommitment #186

Closed
wants to merge 21 commits into from
Closed

Optimize encoded-size of SignedCommitment #186

wants to merge 21 commits into from

Conversation

de-sh
Copy link

@de-sh de-sh commented May 18, 2021 •
edited
Loading

Fixes #128

  • Creates a new, better compressed representation of SignedCommitment
  • Implement parity_scale_codec::Encode
  • Implement parity_scale_codec::Decode

@cla-bot-2021
Copy link

cla-bot-2021 bot commented May 18, 2021

User @de-sh, please sign the CLA here.

@adoerr
Copy link
Contributor

adoerr commented May 19, 2021

@de-sh can you pls sign the contributors agreement (CLA) above?

@de-sh
Copy link
Author

de-sh commented May 19, 2021

@de-sh can you pls sign the contributors agreement (CLA) above?

Hey, I think I did 😅

I am working with @tomusdrw to prepare the PR for merger.

@adoerr
Copy link
Contributor

adoerr commented May 19, 2021

Looks like the Bot thinks otherwise

@de-sh
Copy link
Author

de-sh commented May 19, 2021

The cla marker within the PR checks seem to be marked green.

@de-sh de-sh marked this pull request as ready for review May 30, 2021 20:59
@de-sh
Copy link
Author

de-sh commented May 30, 2021 •
edited
Loading

@tomusdrw seems like the hex literal for compressed version of SignedCommitment, can you check if this is proper.

tomusdrw
tomusdrw reviewed May 31, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Great, thanks!

beefy-primitives/src/commitment.rs Show resolved Hide resolved
beefy-primitives/src/commitment.rs Outdated Show resolved Hide resolved
beefy-primitives/src/commitment.rs Show resolved Hide resolved
beefy-primitives/src/commitment.rs Outdated Show resolved Hide resolved
beefy-primitives/src/commitment.rs Outdated Show resolved Hide resolved
tomusdrw
tomusdrw reviewed Jun 3, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw 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, few improvements noted.

beefy-primitives/src/commitment.rs Outdated Show resolved Hide resolved
beefy-primitives/src/commitment.rs Outdated Show resolved Hide resolved
beefy-primitives/src/commitment.rs Show resolved Hide resolved
@de-sh
Copy link
Author

de-sh commented Jun 27, 2021 •
edited
Loading

Hey, so I wrote a test to ensure that future contributors don't perform updates that remove constraints regarding off-eight signature(len>8 & len%8 != 0). Could add it here if you want, till there, resides in this commit.

Do you feel we are ready to merge?

tomusdrw
tomusdrw approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm! Have you considered using bitvec crate?

@de-sh
Copy link
Author

de-sh commented Jun 28, 2021

Not really, I was trying to achieve a simple implementation without adding external dependencies. Would that have been the right way to go?

@tomusdrw
Copy link
Contributor

No, I was asking just out of curiosity. I guess it would simplify bit conversion quite a lot, but since it's already solved I'm happy to avoid the extra dependency.

@tomusdrw tomusdrw added the Z-breaking This change is breaking backward compatibiltiy on protocol level. label Jul 13, 2021
@tomusdrw tomusdrw mentioned this pull request Jul 18, 2021
Closed
@seunlanlege
Copy link

seunlanlege commented Nov 17, 2021 •
edited
Loading

looks like this pr needs to go to substrate, you want to handle that @de-sh

@adoerr
Copy link
Contributor

adoerr commented Nov 17, 2021

This has not been merged on purpose so far. Merging this will break Snowforks's SnowBridge

@seunlanlege
Copy link

Merging this will break Snowforks's SnowBridge

They're big boys, they can handle it 😂 @musnit

@musnit
Copy link

musnit commented Nov 17, 2021

Merging this will break Snowforks's SnowBridge

They're big boys, they can handle it 😂 @musnit

:D yeh once this is finalized and ready we can start working on including it

@de-sh
Copy link
Author

de-sh commented Nov 17, 2021

looks like this pr needs to go to substrate, you want to handle that @de-sh

Can you lemme know what I can do?

@tomusdrw
Copy link
Contributor

tomusdrw commented Dec 1, 2021

Can you lemme know what I can do?

@de-sh apologies for the delay. Could you please try to apply exactly the same code changes as in this PR, but to the https://github.com/paritytech/substrate/ repository? BEEFY code resides now there. I'm willing to fast-track this change this time to make sure we merge this before any real-world deployment.

de-sh added a commit to de-sh/substrate that referenced this pull request Dec 2, 2021
@de-sh
Include code from paritytech/grandpa-bridge-gadget#186
104a0cd
paritytech-processbot bot pushed a commit to paritytech/substrate that referenced this pull request Dec 3, 2021
@de-sh @tomusdrw
Optimize encoded-size of SignedCommitment (#10409)
e9ebf0c 
* Include code from paritytech/grandpa-bridge-gadget#186

* Cargo fmt commitment.rs

* Make changes suggested by @tomusdrw

Co-authored-by: Tomasz Drwięga <tomasz@parity.io>
@de-sh
Copy link
Author

de-sh commented Dec 7, 2021

Closing in favor of paritytech/substrate#10409

@de-sh de-sh closed this Dec 7, 2021
@tomusdrw
Copy link
Contributor

tomusdrw commented Dec 7, 2021

Thank you @de-sh!!!

AurevoirXavier pushed a commit to darwinia-network/substrate that referenced this pull request Feb 7, 2022
@de-sh @tomusdrw @AurevoirXavier
Optimize encoded-size of SignedCommitment (paritytech#10409)
50a49e8 
* Include code from paritytech/grandpa-bridge-gadget#186

* Cargo fmt commitment.rs

* Make changes suggested by @tomusdrw

Co-authored-by: Tomasz Drwięga <tomasz@parity.io>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@de-sh @tomusdrw @grishasobol
Optimize encoded-size of SignedCommitment (paritytech#10409)
44cda9c 
* Include code from paritytech/grandpa-bridge-gadget#186

* Cargo fmt commitment.rs

* Make changes suggested by @tomusdrw

Co-authored-by: Tomasz Drwięga <tomasz@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adoerr adoerr adoerr left review comments

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees
No one assigned
Labels
Z-breaking This change is breaking backward compatibiltiy on protocol level.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use bitfield in SignedCommitment for encoded-size optimisation.
5 participants
@de-sh @adoerr @tomusdrw @seunlanlege @musnit