INDY-2138: add a documentation for a new PBFT View Change #1431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Toktar
requested review from
lampkin-diet,
ashcherbakov and
skhoroshavin
as code owners
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
docs/source/audit_ledger.md
Outdated
| @@ -47,6 +47,10 @@ In particular, it needs to know: | |||
| - Contains in the last transaction from Audit ledger | |||
| - Primaries for each protocol instance | |||
| - Contains in the last transaction from Audit ledger | |||
| - NodeReg - list of active nodes to select next primary | |||
There was a problem hiding this comment.
Suggested change
| - NodeReg - list of active nodes to select next primary | |
| - NodeReg - list of active nodes from which to select next primary |
docs/source/audit_ledger.md
Outdated
| @@ -47,6 +47,10 @@ In particular, it needs to know: | |||
| - Contains in the last transaction from Audit ledger | |||
| - Primaries for each protocol instance | |||
| - Contains in the last transaction from Audit ledger | |||
| - NodeReg - list of active nodes to select next primary | |||
| - Contains in the last transaction from Audit ledger | |||
There was a problem hiding this comment.
Probably
Suggested change
| - Contains in the last transaction from Audit ledger | |
| - Contained in the last transaction from Audit ledger |
docs/source/audit_ledger.md
Outdated
| @@ -166,6 +176,10 @@ it will stash all corresponding 3PC messages and will apply them when catch-up f | |||
| all ledgers are correctly caught up till the same 3PC Batch and the pool state is properly recovered (see next section), | |||
| the node can achieve the same state as other nodes. | |||
|
|
|||
| ### View Change | |||
There was a problem hiding this comment.
According to surrounding headings...
Suggested change
| ### View Change | |
| #### View Change |
docs/source/message_requests.md
Outdated
| ###ViewChange messages | ||
|
|
||
| Loose a NewView message from primary. It is necessary to finish a view change. A node requests NewView messages from all nodes in NEW_VIEW_TIMEOUT after a view change started. |
There was a problem hiding this comment.
Suggested change
| Loose a NewView message from primary. It is necessary to finish a view change. A node requests NewView messages from all nodes in NEW_VIEW_TIMEOUT after a view change started. | |
| Lost NewView message will prevent view change from finishing successfully. A node requests NewView messages from all nodes in NEW_VIEW_TIMEOUT after a view change started. |
docs/source/message_requests.md
Outdated
|
|
||
| Loose a NewView message from primary. It is necessary to finish a view change. A node requests NewView messages from all nodes in NEW_VIEW_TIMEOUT after a view change started. | ||
| - If an answer is received from a primary is received the node uses them and finishes the view change. |
There was a problem hiding this comment.
Suggested change
| - If an answer is received from a primary is received the node uses them and finishes the view change. | |
| - If an answer is received from expected primary node uses it and finishes the view change. |
docs/source/message_requests.md
Outdated
| - Else it uses a quorum (f+1) of NewView responses from other nodes, finishes the view change and starts catchup. | ||
|
|
||
| Loose a ViewChange message from primary. It is necessary to finish a view change. A node requests missing ViewChange messages from all nodes after receiving a NewView message. |
There was a problem hiding this comment.
Suggested change
| Loose a ViewChange message from primary. It is necessary to finish a view change. A node requests missing ViewChange messages from all nodes after receiving a NewView message. | |
| Lost ViewChange message can prevent view change from finishing successfully. A node requests missing ViewChange messages from all nodes after receiving a NewView message. |
docs/source/message_requests.md
Outdated
| Loose a NewView message from primary. It is necessary to finish a view change. A node requests NewView messages from all nodes in NEW_VIEW_TIMEOUT after a view change started. | ||
| - If an answer is received from a primary is received the node uses them and finishes the view change. | ||
| - Else it uses a quorum (f+1) of NewView responses from other nodes, finishes the view change and starts catchup. |
There was a problem hiding this comment.
I'd prefer to use "otherwise" instead of "else", but this probably is subjective...
| deactivate Node4 | ||
| note right Node4 | ||
| <b>VIEW_CHANGE_ACK contains: | ||
| - <b>view_no</b> <i>(number of future view change), |
There was a problem hiding this comment.
Suggested change
| - <b>view_no</b> <i>(number of future view change), | |
| - <b>view_no</b> <i>(future view number), |
| end note | ||
|
|
||
|
|
||
| opt If Node1 preprepared less then the previous view prepare certificate |
There was a problem hiding this comment.
Suggested change
| opt If Node1 preprepared less then the previous view prepare certificate | |
| opt If Node1 preprepared less than prepare certificate from previous view |
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
|
test this please |
2 similar comments
|
test this please |
|
test this please |
| @@ -29,10 +29,6 @@ note right Node4 | |||
| - Replicas do not generate, send or process any 3PC messages | |||
| - All 3PC messages are stashed (in received order) | |||
There was a problem hiding this comment.
Suggested change
| - All 3PC messages are stashed (in received order) | |
| - All 3PC messages as well as checkpoints and view-change-related messages are stashed (in received order) |
| @@ -305,36 +301,6 @@ loop For each replica | |||
| end loop | |||
|
|
|||
There was a problem hiding this comment.
Instead of primaries = primaries from last audit txn we need to say
- node_reg is recovered from the audit ledger
- primaries are calculated based on recovered node registry (using a node registry as it was at the beginning of previous view).
| note over Node1, Node4 | ||
| <b>Node1 send VIEW_CHANGE </b> | ||
| end note | ||
|
|
There was a problem hiding this comment.
We increment view_no here
| - <b>preprepared</b> <i>(list of BatchId(view_no, pp_view_no, pp_seq_no, pp_digest)) | ||
| - <b>checkpoints</b> <i>(list of checkpoint messages) | ||
| end note | ||
|
|
There was a problem hiding this comment.
We schedule a timer to check if view change has finished
| @@ -0,0 +1,129 @@ | |||
| @startuml | |||
There was a problem hiding this comment.
It would be great to add a doc with a text description of a View Change process.
It's important to mention in this doc how Primary Selection logic works and how it uses node_reg.
…nto pbft-doc-update
Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>
Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>
INDY-2138: PBFT diagrams update
ashcherbakov
approved these changes
Dec 20, 2019
| Node1 -> Node4: INSTANCE_CHANGE(viewNo=3) | ||
|
|
||
| Node2 -> Node1: INSTANCE_CHANGE(viewNo=3) | ||
| Node4 -> Node1: INSTANCE_CHANGE(viewNo=3) |
There was a problem hiding this comment.
I think it also makes sense to add
Node2 -> Node3: INSTANCE_CHANGE(viewNo=3)
Node2 -> Node4: INSTANCE_CHANGE(viewNo=3)
Node4 -> Node2: INSTANCE_CHANGE(viewNo=3)
Node4 -> Node3: INSTANCE_CHANGE(viewNo=3)
There was a problem hiding this comment.
Actually there is a loop for Node 2 and 4 there, but I agree that explicit arrows would be more clear.
| <b>Start a View Change</b> | ||
| end note | ||
|
|
||
| Node1 -> Node1: Start View Change to viewNo=3 |
There was a problem hiding this comment.
I think it also makes sense to add
Node2 -> Node2: Start View Change to viewNo=3
Node4 -> Node4: Start View Change to viewNo=3
There was a problem hiding this comment.
Actually there is a loop for Node 2 and 4 there, but I agree that explicit arrows would be more clear.
| - Individual transaction author agreements can be disabled by setting retirement date using `TRANSACTION_AUTHOR_AGREEMENT` transaction. | ||
| Retirement date can be in future, in this case deactivation of agreement won't happen immediately, it will be automatically deactivated at required date instead. | ||
|
|
||
| - Retirement date value may have any precision (seconds, milliseconds, etc.) |
There was a problem hiding this comment.
Since we expect integer POSIX timestamp precision is limited to seconds actually
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: toktar renata.toktar@dsr-corporation.com