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

IF : Extra padding is added to the hs_bitset when constructing a quorum_certificate from a quorum_certificate_message #1626

Closed
Tracked by #1508
systemzax opened this issue Sep 13, 2023 · 5 comments · Fixed by #1616
Assignees

Comments

@systemzax
Copy link
Member

Printing a quorum_certificate bitset when created :
Id : PVT_BLS_r4ZpChd87ooyzl6MIkw23k7PRX8xptp7TczLJHCIIW88h/hS finalizers 21 000000111111111111111

Printing a quorum_certificate bitset when received :
Id : PVT_BLS_/l7xzXANaB+GrlTsbZEuTiSOiWTtpBoog+TZnirxUUSaAfCo finalizers 32 00000000000000000111111111111111

The message format seems to (correctly) encode the bitset using the minimum required number of unsigned integers to represent the active finalizer set votes, but the decoding should also trim the extra leading zeroes.

@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 13, 2023

If I understand correctly, and the first bitset is for 21 finalizers and the second for 32 finalizers, I think the leading zeroes are meaningful, as the first output is exactly 21 binary digits and the second exactly 32, so in my opinion they should not be trimmed.

@heifner
Copy link
Member

heifner commented Sep 13, 2023

If I understand correctly, and the first bitset is for 21 finalizers and the second for 32 finalizers, I think the leading zeroes are meaningful, as the first output is exactly 21 binary digits and the second exactly 32, so in my opinion they should not be trimmed.

The issue is that the conversion to "block" form out of boost::dynamic_bitset is filling in the extra bits of our uint32_t with zeros. Then on the other end we create the boost::dynamic_bitset from that set of 32 bits making it now 32 instead of 21.

We either need to implicitly know the finalizer set size (seems like we should know that) or pass along in the message how many of the provided bits are relevant (21 vs 32).

@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 13, 2023

Oh thanks, I get it now. boost::dynamic_bitset does know its size, which appears to be lost when serialized, right?
It is because active_finalizers in quorum_certificate_message is defined as std::vector<unsigned_int> and not boost::dynamic_bitset (which fc probably does not know how to serialize).

@heifner
Copy link
Member

heifner commented Sep 13, 2023

Oh thanks, I get it now. boost::dynamic_bitset does know its size, which appears to be lost when serialized, right?

Correct.

@heifner
Copy link
Member

heifner commented Sep 13, 2023

Change boost::dynamic_bitset to use uint8_t to be less wasteful and to avoid zigzag encoding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
5 participants