-
Notifications
You must be signed in to change notification settings - Fork 32
Conversation
Signed-off-by: Adam Ludvik <[email protected]>
text/0000-consensus-api.md
Outdated
[summary]: #summary | ||
|
||
This RFC describes a new Consensus API for supporting both the existing | ||
_Nakamoto-style_ consensus algorithms as _elected leader_ algorithms such as |
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.
typo?
:s/as/as well as
text/0000-consensus-api.md
Outdated
|
||
This RFC describes a new Consensus API for supporting both the existing | ||
_Nakamoto-style_ consensus algorithms as _elected leader_ algorithms such as | ||
rBFT. |
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.
PBFT is the general case. rBFT is probably the first PBFT-like consensus we'll add.
text/0000-consensus-api.md
Outdated
|
||
These observations led to the creation of the _Consensus Engine_ abstraction, | ||
which can be used to implement any consensus algorithm that is either an | ||
_elected leader_ algorithm or a _Nakamoto-style_ 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.
The hyperledger architecture paper on consensus breaks these methods into lottery (PoET, POW, etc) and voting (PBFT, RBFT, RAFT, etc) based elections. The use of elected leader could be misunderstood. (see https://www.hyperledger.org/wp-content/uploads/2017/08/Hyperledger_Arch_WG_Paper_1_Consensus.pdf)
Signed-off-by: Adam Ludvik <[email protected]>
This is to be consistent with the Hyperledger Language Signed-off-by: Adam Ludvik <[email protected]>
Pushed some commits to address @dcmiddle comments |
text/0000-consensus-api.md
Outdated
|
||
The Consensus Engine interface and implementations of the interface should be | ||
reusable in other applications. Specifically, the interface should support | ||
non-blockchain applications. |
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 too vague for me. Can you give an example of why and what non-blockchain app needs this?
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.
For example, it would be nice if we can re-use consensus engines to implement a distributed oracle.
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.
Addressed
|
||
| Method | Description | | ||
| --- | --- | | ||
| GetSetting(setting) -> data | Read the current value of the setting. Supports R4. | |
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.
Is this a convenience method wrapping GetState(address) or does it imply settings outside of global state?
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.
yes
text/0000-consensus-api.md
Outdated
| Method | Description | | ||
| --- | --- | | ||
| OnMessageReceived(message) | Called when a new consensus message is received | | ||
| OnNewBlockReceived(block) | Called when a new block is received and validated | |
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.
Does "and validated" imply that validator will validate transactions before consensus considers whether the block is valid or has precedence?
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 was what the original design called for. The API was changed to include a CheckBlocks()
method which signals to the validator that it should "check that the block can be committed" which allows the validator to send blocks to consensus before they are validated. At this time however, blocks are still validated prior to sending them to consensus. The alternative requires two new feautres: 1. the ability to process all consensus-specific transactions separately from regular transactions and 2. the ability to read, write, and verify state sub-trees.
text/0000-consensus-api.md
Outdated
| Method | Description | | ||
| --- | --- | | ||
| OnMessageReceived(message) | Called when a new consensus message is received | | ||
| OnNewBlockReceived(block) | Called when a new block is received and validated | |
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 you envision sending the entire block or just the block header?
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.
In the end, everything but batches are sent.
| --- | --- | | ||
| GetSetting(setting) -> data | Read the current value of the setting. Supports R4. | | ||
| GetState(address) -> data | This is needed to read values from arbitrary addresses used by consensus (eg., validator registry). Supports R5. | | ||
| GetBlock(block_id) -> block | Retrieve consensus-related information about a block. | |
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.
Will this return the entire block or just the block header?
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.
In the end, everything but batches are sent.
The Block Publisher handles the creation of blocks. It is directed by the | ||
Consensus Engine on when to do this. When a block is finalized, it is forwarded | ||
to the Block Validator. | ||
|
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.
Block Store and Block Cache from the diagram are not defined here. No need for unnecessary work if the definitions remain the same as here: https://sawtooth.hyperledger.org/docs/core/releases/latest/architecture/journal.html.
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 are not very relevant to the discussion and are superseded by the block manager.
Signed-off-by: Adam Ludvik <[email protected]>
| --- | --- | | ||
| Startup() | Startup and synchronize with any peers if necessary. | | ||
| Shutdown() | Shutdown and notify peers if necessary. | | ||
|
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.
How does one register a consensus engine with the validator?
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 more detail than I think belongs in the RFC and I will include it in the documentation.
This RFC should be updated to match the implementation prior to FCP. |
Signed-off-by: Adam Ludvik <[email protected]>
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.
2 comments
1 apparent error (read vs. read/write)
of the fork-resolution logic. | ||
|
||
Examples of _lottery_ consensus algorithms include Proof-of-Work (PoW) | ||
and Proof-of-Elapsed-Time (PoET). |
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.
Just a suggestion--I would add "Also known as Nakamoto-style consensus"
coordinate consensus. | ||
|
||
Examples of _voting_ algorithms include PBFT, rBFT, Tendermint, and | ||
Raft. |
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.
Just a suggestion--I would add "Also known as classical consensus"
**R5 - Read/Write Access to Global State** | ||
|
||
PoET depends on tracking authorized block publishers in global state and | ||
therefore requires read access to a PoET specific namespace. |
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.
Where does the "Write" (from "Read/Write Access") come in here. I think you mean "read/write" access on line 128.
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 should just say Read
text/0000-consensus-api.md
Outdated
This design borrows some ideas from the [Tendermint ABCI][tendermint] and the | ||
[Parity Consensus Engine][parity] interface. | ||
|
||
[tendermint]: https://tendermint.readthedocs.io/en/master/app-development.html#abci-design) |
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.
Dead link; https://tendermint.com/docs/spec/abci/ maybe?
This is not intended to be part of the API. Signed-off-by: Adam Ludvik <[email protected]>
Signed-off-by: Adam Ludvik <[email protected]>
Signed-off-by: Adam Ludvik <[email protected]>
I move that this RFC enter Final Comment Period with a disposition to |
Checkboxes for core team:
|
Signed-off-by: Adam Ludvik [email protected]