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

Roll-back BLS #2335

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Roll-back BLS #2335

merged 1 commit into from
Sep 21, 2022

Conversation

colibrishin
Copy link
Contributor

@colibrishin colibrishin commented Sep 21, 2022

TLDR

  • It causes a lot of conflict between in Unity Support, and It is now becoming a blocker of testing in preview net.
  • Until the PBFT is separated from Libplanet project, BLS support is postponed.

The reason for BLS implementation

The BLS signature is faster than the current libplanet implementation of secp256k11, and it can reduce the verification step by the aggregation of signatures. The initial decision was made because fast cryptography is needed for handling multiple signature verification in a limited time (PreCommit, PreVote, using round short as possible2.) The interoperability is also part of the reason why BLS is needed (One simple example is an alternative method for random seed by using drand. the random created from drand beacon chain, it needed to be verified whether the random is valid.3)

Timeline

The goal of this sprint was implementing BLS for me, and it was almost completed in a sense of creating a binding of C# with multi-platform support and preparing for interfacing in libplanet.

C# binding

There was no multiplatform supporting or implemented C# BLS library, so it has to be made for bringing it to libplanet. Decided to create a C# binding based on herumi/bls which is a BLS Signature in C. There was a time of testing, however, most of the sprint time is used for creating a C# binding (The multithread-safeness, RPATH in CMake, and AutoNativeImport) and it was necessary to make it work.4 Despite of worries, making a multi-platform support C# binding was done successfully.5

Interfacing

For interfacing, following changes are made: BoundPeer, creates a new Address and retyping PBFT-related public key from PublicKey to BlsPublicKey, changing ITransport to take multiple types of a public key. It seems like to pass the unit test, but the unity test.

Unity support

The problem arises with Unity support. For binding to work, it requires System.Reflection.Emit to dynamically load C library. However, due to IL2CPP, it is limited to being used.6 The mistake were made from here. I was interpreting "the build is passing, and the test is failing" as it would be okay if it doesn't use BLS in unity directly. It appeared to be wrong and failed to build the NineChronicle (unity part.)

Separation

For solving problem, the BLS, and PBFT-related consensus part has to be separated from libplanet (e.g., RocksDBStore.) For that, It requires some structural reorganization and that seems like It would require another sprint cycle to be done. (Define a new type for placeholding the Vote data in libplanet, creates a new project, and move PBFT implementation into it. (Context, BlsPublicKey, BlsPrivateKey, ConsensusReactor, ConsensusContext, Vote, VoteSet etc...) and resolves circular reference between Libplanet.Net and the new consensus project.)

For Previewnet

I was thinking that BLS might delay the whole test phase of PBFT. It works without BLS, so the roll-back decision has been made without hesitation. it is postponed until the separation is done.

Related or Following closed issues/PRs/repository

Footnotes

  1. https://github.com/planetarium/libplanet/pull/2223 Section. How Fast it is?, Note that it is not the time of libSecp256k1 which is used in NineChronicle.Headless.

  2. https://github.com/planetarium/libplanet/blob/ec354eb5b5fc6295bab444ba7300f309fed0bf9a/Libplanet.Net/Consensus/Context.cs#L81-L86

  3. https://drand.love/docs/cryptography/#pairing-based-cryptography

  4. On second thought, the work-through time of RPATH and importing the native library dynamically could have been shorter than now if I have seen this document.

  5. Note that it is not "true-C#" implementation. Creating own library without proper knowledge is considered dangerous.

  6. https://docs.unity3d.com/Manual/ScriptingRestrictions.html

@colibrishin colibrishin added the pbft Related to PBFT consensus label Sep 21, 2022
@colibrishin colibrishin self-assigned this Sep 21, 2022
@pull-request-quantifier-deprecated

This PR has 1916 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!


Quantification details

Label      : Extra Large
Size       : +317 -1599
Percentile : 100%

Total files changed: 65

Change summary by file extension:
.md : +0 -56
.cs : +317 -1542
.csproj : +0 -1

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detected.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.


Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@colibrishin colibrishin marked this pull request as ready for review September 21, 2022 08:22
@colibrishin colibrishin merged commit ec354eb into planetarium:pbft Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra Large pbft Related to PBFT consensus
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants