-
Notifications
You must be signed in to change notification settings - Fork 96
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
Readme updates #64
Readme updates #64
Conversation
README.md
Outdated
|
||
* Other language implementations | ||
|
||
* [Go ](https://github.com/anthdm/hbbft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Go ] -> [Go]
README.md
Outdated
["Honey Badger of BFT Protocols"](https://eprint.iacr.org/2016/199.pdf) | ||
in Rust. This is a modular library of consensus. There are | ||
[examples](./examples/README.md) illustrating the use of this algorithm. | ||
Welcome to a [Rust ](https://www.rust-lang.org/en-US/)library of the Honey Badger Byzantine Fault Tolerant (BFT) consensus algorithm. The research and protocols for this algorithm are explained in detail in "[The Honey Badger of BFT Protocols](https://eprint.iacr.org/2016/199.pdf)" by Miller et al. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the space outside [Rust ]
.
CONTRIBUTING.md
Outdated
All pull requests must include: | ||
* A clear, readable description of the purpose of the PR | ||
* A clear, readable description of changes | ||
* A title that includes (Fix), (Feature), or (Refactor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already broke the rule in this very PR. 😛
I'm still skeptical about it, and my experience with mandatory PR/commit title formats was never great:
- The only projects I found that follow this one are poa-test-setup and poa-dapps-validators, and even they have already extended the list of allowed keywords ("Update").
- The RFC itself is still a draft. Also, it refers only to DApps; hbbft is not a DApp.
- The only way to get a whole team to follow these rules is to check them in CI. But then it's even worse when you have something that doesn't quite fit one of the three categories. This PR here, for example, should really be in a "Docs" category if anything. Although I don't find "(Docs) Readme updates" much more readable than just "Readme updates".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. At this stage in the project I think it's appropriate to remove the Contributing section until it is more fleshed out. Once I explore options and get feedback I will create a centralized Contributing file.
CONTRIBUTING.md
Outdated
* A single commit message for one specific fix or feature. A separate PR should be made for each specific change. | ||
* Any additional concerns or comments (optional) | ||
|
||
All accepted and completed PRs are updated in the Wiki documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we already have a Wiki entry for hbbft? I'd only add this sentence once we do.
(Also: Should we have one? If we start duplicating the documentation, it will start to go out of sync.)
README.md
Outdated
**Note:** This library is a work in progress and parts of the algorithm are still in development. | ||
|
||
## What is Honey Badger? | ||
The Honey Badger consensus algorithm allows nodes in a distributed, potentially asynchronous environment (decentralized databases and blockchains) to achieve agreement on transactions. The agreement process does not require a leader node, tolerates corrupted nodes, and makes progress in adverse network conditions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be misunderstood as "decentralized databases and blockchains" being the environment — if you're inside the blockchain, e.g. you're writing a DApp, you actually have a perfectly synchronous environment to work with —; the reader should understand that this can be used to build those kinds of systems on top of an asynchronous environment, e.g. nodes on the internet that can have connection problems.
We could just move it to the end of the paragraph: "Example use cases are decentralized databases and blockchains."
README.md
Outdated
|
||
### Algorithms | ||
|
||
All algorithms in the protocol are modular and usable. Encryption to provide censorship resistance is currently in process for the top level Honey Badger algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "and usable independently"?
README.md
Outdated
|
||
## Getting Started | ||
|
||
This Rust library requires a distributed network environment to function. Details on network requirements will be published in the [Rust package registry](https://crates.io/) once core algorithms are complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that crates.io displays exactly the contents of the README.md. And there's no need to wait until we publish it there; I'd just put a "TBD" here for now. I'll try to come up with a very minimal usage example soon.
README.md
Outdated
|
||
$ cargo build [--release] | ||
* [Erlang](https://github.com/helium/erlang-hbbft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also Andrew Miller's original implementation:
* [Python](https://github.com/amiller/HoneyBadgerBFT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unfinished one:
* [Rust](https://github.com/rphmeier/honeybadger)
README.md
Outdated
# About | ||
Welcome to a [Rust](https://www.rust-lang.org/en-US/) library of the Honey Badger Byzantine Fault Tolerant (BFT) consensus algorithm. The research and protocols for this algorithm are explained in detail in "[The Honey Badger of BFT Protocols](https://eprint.iacr.org/2016/199.pdf)" by Miller et al. | ||
|
||
This documentation is designed for Rust developers looking to use a resilient consensus algorithm on a distributed network. Following is an overview of HoneyBadger BFT and basic instructions for getting started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd omit this sentence. It already says above that it's a Rust library, and use cases are listed below.
README.md
Outdated
|
||
- [ ] Dynamic Honey Badger (adding and removing nodes in a live network environment) | ||
|
||
- [ ] Networking example to detail Honey Badger implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could link to #41 (first one) and #47 (comment) (second one); once we've agreed on what the example should do, we'll also open an issue for the third one.
README.md
Outdated
[![Build Status](https://travis-ci.com/poanetwork/hbbft.svg?branch=master)](https://travis-ci.com/poanetwork/hbbft) | ||
[![Gitter](https://badges.gitter.im/poanetwork/hbbft.svg)](https://gitter.im/poanetwork/hbbft?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge) | ||
|
||
# About | ||
Welcome to a [Rust](https://www.rust-lang.org/en-US/) library of the Honey Badger Byzantine Fault Tolerant (BFT) consensus algorithm. The research and protocols for this algorithm are explained in detail in "[The Honey Badger of BFT Protocols](https://eprint.iacr.org/2016/199.pdf)" by Miller et al. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a list of points in which we deviate from the paper. Currently those would be, I think:
- We use pairing-based cryptography instead of Gap Diffie-Hellman groups. But the principles are the same, and I think even Andrew Miller's Python implementation does that.
- We added a
Terminate
message to Binary Agreement; otherwise the algorithm would have to run (or at least stay in memory) forever: Add aTerminate
message to ABA? #55 - We added a
Conf
message to Binary Agreement: Fix Common Coin use inAgreement
. #37. (The Python implementation will probably also do that.) - We return some additional information from Subset and Honey Badger, namely which node had input which item/transaction.
Later, we'll implement the dynamic variant that can add and remove nodes (that's not in the paper), and maybe change the "coin schedule" (output fixed 0, fixed 1, then flip the coin, if it actually improves efficiency), and others.
(@vkomenda: Can you think of other points in which we differ from the paper?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afck also removed the requirement to have a trusted dealer in Distributed Key Generation (DKG) and, consequently, in the Common Coin algorithm. In hbbft
, DKG is decentralized and runs without a trusted dealer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. @afck can your provide a little more context to explain the reasoning behind the modifications? These may be obvious but I want to convey why the choices were made and what improvements result from these changes. Thanks!
-
Was pairing-based cryptography chosen to increase security, efficiency and/or implementation ease (Rust elliptic curve library)? Or other reasons?
-
What are potential uses for returned data from Subset & HB?
-
Are there any performance or other potential tradeoffs to run DKG without a trusted dealer? Was this choice made to enhance security/mitigate a potential single point of attack?
I may also add a table which shows the paper's naming conventions compared to this implementation for easy reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust elliptic curve library
Yes, that was the main reason. If we'd want to switch to e.g. the same elliptic curves that the Python implementation uses, we'd first have to create Rust bindings for some cryptography C library.
What are potential uses for returned data from Subset & HB?
E.g. you could detect malicious behavior: If a node inputs invalid transactions it may be useful to be able to identify and block or disconnect from that node.
DKG without a trusted dealer?
Requiring a trusted dealer severely limits its usefulness: In a distributed network you usually don't want a single person to know everyone's secret keys. The paper itself even mentions dealerless DKG, it just doesn't go into details because it goes a bit beyond its scope. The Python implementation has an open issue for it. The Erlang implementation has dealerless DKG, too.
CONTRIBUTING.md
Outdated
https://github.com/poanetwork/hbbft | ||
2. Create a feature branch | ||
3. Write tests to cover the work | ||
4. Commit changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before committing changes the contributor has to ensure the CI rules are satisfied, in particular, formatting is correct and all CI tests pass:
Line 21 in 1436d85
script: |
Otherwise they may be waiting for acceptance of a PR that doesn't pass CI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! We should definitely link to the .travis.yml
here, and explain that you have to use the same Rust, Clippy and Rustfmt versions as configured in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I will be sure to add.
README.md
Outdated
|
||
``` | ||
$ cargo run --example simulation --features=serialization-serde -- -h | ||
$ cargo run --example simulation -h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed serialization-serde - unsure if any other flags are used at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. The flag serialization-serde
has been removed. Wherever you see --features=serialization-serde
, please remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
Looks good to me, too, but there seems to be a merge conflict. |
I made changes according to recommendations and removed serde from example. I envision the order of items changing (getting started section will move up) in the future.