-
Notifications
You must be signed in to change notification settings - Fork 20
Support for reconcile confirmation against the in-memory canonical chain #174
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Co-authored-by: alexey semenyuk <[email protected]> Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
EnriqueL8
left a comment
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, looks really good - a few questions and suggestions but thanks for all the comments in the flow
Signed-off-by: Chengxuan Xing <[email protected]>
… reconcile-confirmation
Signed-off-by: Chengxuan Xing <[email protected]>
Co-authored-by: Enrique Lacal <[email protected]> Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
|
@Chengxuan thanks for walking me through this. Here is the diagram that I sketched as we discussed it. Wanted to share it here with a couple of notes to verify my understanding
Where the main intent of the algorithm is highlighted within the grey box - i.e. to construct a contiguous linked list of blocks by chopping away at the end of the confirmations queue, filling the gap with a block from the "canonical chain or directly from the blockchain and verifying it as a valid link in the linked list. Prereqs for entering this grey box is (and the purpose of the steps before this box)
The early steps in the diagram ....
are not essential to the functional behaviour of the algorithm but are an optimization to allow us to end early in the case where we have already reached sufficient level of confirmation but previous iterations of the algorithm did not conclude that. Most likely because the config has been altered (by reducing the number of required confirmations) between 2 iterations. Have I captured a correct understanding? If so, hope this write up helps explain it to others and maybe helps inform some code structure, and naming / refactoring |
|
Thanks @hosie for sharing the diagram. The description makes sense and is an accurate summary. I renamed For the diagram, I added some annotations to give others more context.
|
Signed-off-by: John Hosie <[email protected]>
Signed-off-by: John Hosie <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
a50c11b to
e0ed0c0
Compare
| // if that block is removed, it means the chain is not stable enough for the logic | ||
| // to generate a valid confirmation list | ||
| // therefore, we report a fork with no confirmations | ||
| return nil, i18n.NewError(ctx, msgs.MsgFailedToBuildConfirmationQueue) |
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 like clarity that is is considered an error condition.
| targetBlockNumber := txBlockInfo.BlockNumber.Uint64() + targetConfirmationCount | ||
|
|
||
| // Check if the in-memory partial chain has caught up to the transaction block | ||
| chainTail := bl.canonicalChain.Back().Value.(*ffcapi.MinimalBlockInfo) |
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 did a code test to confirm this panics if the list is empty - couldn't see any comments for why it's guaranteed not to be emby
bl.canonicalChain.Back().ValueThere 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 fix the panic here. Thanks for spotting it
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
… reconcile-confirmation
Signed-off-by: Chengxuan Xing <[email protected]>



Requires hyperledger/firefly-transaction-manager#148
This PR introduced a new method,
ReconcileConfirmationsForTransaction,which can be used to directly calculate the confirmation map of a given transaction.The existing confirmation logic in FFTM has already done a great job in the following areas:
Therefore, this PR was built on top of it with the following additional features and refinements:
Algorithm is peer-reviewed with @hosie , a summary can be found here: #174 (comment)