Add commit phase to consensus algorithm #534
Conversation
|
Looks fine, @erikzhang. |
|
Online here https://neocompiler.io/#/ecolab |
|
Looks nice to me, a great step towards a stronger network. |
| localNode.Tell(new LocalNode.Relay { Inventory = block }); | ||
| context.State |= ConsensusState.BlockSent; | ||
| ConsensusPayload payload = context.MakeCommit(); | ||
| Log($"send commit"); |
There was a problem hiding this comment.
Extra information is also nice here, such as view and height,. good for drawing logs.
There was a problem hiding this comment.
I spent some time looking over the consensus code and these changes and it generally looks good to me.
There was one question that I had though about nodes crashing after sending prepare request but just before sending commit; then I realized, that is why regeneration will be added after this change to fix that issue.
I plan to test these changes along with my MemoryPool changes tonight or perhaps tomorrow.
| @@ -136,13 +148,31 @@ private void Log(string message, LogLevel level = LogLevel.Info) | |||
|
|
|||
| private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message) | |||
| { | |||
| if (context.State.HasFlag(ConsensusState.CommitSent)) | |||
| return; | |||
There was a problem hiding this comment.
We do not have recover message. What's would happen if half of the CN was restarted when others are in this step
There was a problem hiding this comment.
Yeah, it has to have the regeneration message. I noticed the same thing as well, but believe they want to make the change in a separate PR on top of this.
There was a problem hiding this comment.
If it will come in other PR, it's fine
There was a problem hiding this comment.
I still prefer PR #426, @erikzhang.
That PR is more complete and already tested.
The only different is, basically, the partial signature merged with Payload Witness.
There was a problem hiding this comment.
I will save the state of the commit phase on the hard disk in the next pr. If the node restarts and finds the state of the commit phase last time, it will automatically recover from this.
There was a problem hiding this comment.
Erik, the Regeneration phase is also important for replying to nodes that asks for change_view (or any other payload that is not expected during commit phase). Those that receive the Regeneration Payload should be able to verify that commit was already achieved.
There was a problem hiding this comment.
This point was not resolved here. However, it will be further discussed in the next PR to be merged into #547.
|
@erikzhang, for the recovery we gonna need to store the Do you think that we can merge this part and then create the other PRs? |
* getrawtransactionheight Nowadays two calls are need to get a transaction height, `getrawtransaction` with `verbose` and then use the `blockhash`. Other option is to use `confirmations`, but it can be misleading. * Minnor fix * Shargon's tip * modified
|
Note that Erik is now pushing to another branch, different than master... so we will have time to improve the solution in other PR. |
|
Looking nice on consensus-draw diagrams. Complete process is running in few milliseconds on a simple computer. |
* Add commit phase to consensus algorithm (#534) * Add commit phase to consensus algorithm * fix tests * Prevent repeated sending of `Commit` messages * RPC call gettransactionheight (#541) * getrawtransactionheight Nowadays two calls are need to get a transaction height, `getrawtransaction` with `verbose` and then use the `blockhash`. Other option is to use `confirmations`, but it can be misleading. * Minnor fix * Shargon's tip * modified * Allow to use the wallet inside a RPC plugin (#536) * Clean code * Clean code * Minor fix on mempoolVerified * Add MemoryPool Unit tests. Fix bug on initital start of Persisting the Genesis block. * Prevent `ConsensusService` from receiving messages before starting (#573) * Prevent `ConsensusService` from receiving messages before starting * fixed tests - calling OnStart now * Consensus recovery log (#572) * Pass store to `ConsensusService` * Implement `ISerializable` in `ConsensusContext` * Start from recovery log * Fix unit tests due to constructor taking the store. * Add unit tests for serializing and deserializing the consensus context. * Combine `ConsensusContext.ChangeView()` and `ConsensusContext.Reset()` * Add `PreparationHash` field to `PrepareResponse` to prevent replay attacks from malicious primary (#576) * Fixed a problem where `PrepareResponse.PreparationHash` was not assigned. * Load context from store only when height matches * Recover nodes requesting ChangeView when possible (#579) * Fixes bug in `OnPrepareRequestReceived()` * Send `RecoveryMessage` only when `message.NewViewNumber <= context.ViewNumber` * Fix and optimize view changing (#590) * Allow to ignore the recovery logs * Add `isRecovering` (#594) * Fix accepting own prepare request (#596) * Pick some changes from #575. * Fixes `Prefixes` * Restore transactions from saved consensus context (#598) * Refactoring * AggressiveInlining (#606) * Reset Block reference when consensus context is initialized after block persist. (#608) * Change `ConsensusPayload` for compatibility (#609)
No description provided.