-
Notifications
You must be signed in to change notification settings - Fork 19
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]>
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
// Note: should consider have an in-memory map of transaction hash to block for faster lookup | ||
// The extra memory usage of the map should be outweighed by the speed improvement of lookup | ||
// But I saw we have a ffcapi.MinimalBlockInfo struct that intentionally removes the tx hashes | ||
// so need to figure out the reason first |
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.
Are you going to do this figuring out as part of the PR? I did wonder why the TX hash -> block was not a thing
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, would like to, waiting for @peterbroadhurst 's input
) | ||
} | ||
|
||
func (bl *blockListener) validateExistingConfirmations(ctx context.Context, occ *ffcapi.ConfirmationMapUpdateResult, newQueue []*ffcapi.MinimalBlockInfo, existingConfirmations []*ffcapi.MinimalBlockInfo, currentBlock *list.Element, chainHead *ffcapi.MinimalBlockInfo, txBlockInfo *ffcapi.MinimalBlockInfo, targetConfirmationCount uint64) ([]*ffcapi.MinimalBlockInfo, *list.Element) { |
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 function was quite difficult to follow FYI
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.
@EnriqueL8 can you elaborate exactly what's difficult to follow so we can improve it?
e.g. the algorithm used / the comments / variable naming?
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]>
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 diagram below shows the core logic of the function:

Details of the confirmation Map struct can be found in this PR: hyperledger/firefly-transaction-manager#148
Reconciliation logic for an existing confirmations that get passed in
newFork
flag will be set to true in the response accordinglyConfirmation queue for different scenarios: