Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[r2r] Fix for LBC block header deserializing bug #1343

Merged
merged 4 commits into from
Jul 21, 2022
Merged

[r2r] Fix for LBC block header deserializing bug #1343

merged 4 commits into from
Jul 21, 2022

Conversation

borngraced
Copy link
Member

fixes incorrect report for LBC mtp.
ref: #1281

@borngraced borngraced requested a review from shamardy July 15, 2022 13:04
@borngraced borngraced changed the title [r2r] Fix for LBC block header deserializing bug [wip] Fix for LBC block header deserializing bug Jul 15, 2022
@borngraced
fixed failing test &&
a59ce07 
Improved Reader with coin_name for deserializing specific coins like LBC correctly
@borngraced
Copy link
Member Author

NOTE: Normally, adding claim_trie_root to BlockHeader and setting it absolutely None fixes the issue of wrong deserializing for LBC and if we try to set the field with claim_trie_root it still fixes the issue but now to do the later, we can only match version to detect if version is LBC version or not which leads to other coins with same version as LBC to fail deserializing e.g QTUM coins.

So to avoid this, I added an extra optional field (coin_name) to Reader structure which takes the coin name currently used for the op and then now I can match for LBC coin successfully without having any effect on other coins.

I hope this is clear @shamardy

@borngraced
Copy link
Member Author

NOTE: Normally, adding claim_trie_root to BlockHeader and setting it absolutely None fixes the issue of wrong deserializing for LBC and if we try to set the field with claim_trie_root it still fixes the issue but now to do the later, we can only match version to detect if version is LBC version or not which leads to other coins with same version as LBC to fail deserializing e.g QTUM coins.

So to avoid this, I added an extra optional field (coin_name) to Reader structure which takes the coin name currently used for the op and then now I can match for LBC coin successfully without having any effect on other coins.

I hope this is clear @shamardy

and also, I thought it might be very useful to set claim_trie_root correctly instead of using None

@borngraced borngraced changed the title [wip] Fix for LBC block header deserializing bug [r2r] Fix for LBC block header deserializing bug Jul 17, 2022
artemii235
artemii235 requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests must be added in such cases similar to https://github.com/KomodoPlatform/atomicDEX-API/pull/1235/files#diff-df097d6b5341b975c56bec29311f55210db049d784eec49f8ec9d2e5f9a70f9aR799

artemii235
artemii235 requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments.

mm2src/mm2_bitcoin/serialization/src/reader.rs Outdated Show resolved Hide resolved
mm2src/mm2_bitcoin/chain/src/block_header.rs Outdated Show resolved Hide resolved
@borngraced
Copy link
Member Author

Unit tests must be added in such cases similar to https://github.com/KomodoPlatform/atomicDEX-API/pull/1235/files#diff-df097d6b5341b975c56bec29311f55210db049d784eec49f8ec9d2e5f9a70f9aR799

Alright, will do that.

artemii235
artemii235 requested changes Jul 19, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more notes.

mm2src/coins/utxo/rpc_clients.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_standard.rs Outdated Show resolved Hide resolved
mm2src/mm2_bitcoin/serialization/src/reader.rs Show resolved Hide resolved
shamardy
shamardy reviewed Jul 19, 2022
View reviewed changes
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! A few questions/comments.

mm2src/mm2_bitcoin/chain/src/block_header.rs Show resolved Hide resolved
mm2src/mm2_bitcoin/serialization/src/reader.rs Show resolved Hide resolved
@borngraced borngraced changed the title [r2r] Fix for LBC block header deserializing bug [wip] Fix for LBC block header deserializing bug Jul 19, 2022
@borngraced borngraced changed the title [wip] Fix for LBC block header deserializing bug [r2r] Fix for LBC block header deserializing bug Jul 19, 2022
@borngraced
Copy link
Member Author

@artemii235 @shamardy this is ready for another round of review

artemii235
artemii235 approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@shamardy shamardy mentioned this pull request Jul 20, 2022
Open
shamardy
shamardy approved these changes Jul 20, 2022
View reviewed changes
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@artemii235 artemii235 merged commit e48e0af into dev Jul 21, 2022
@artemii235 artemii235 deleted the lbc_deserializing_bug branch July 21, 2022 06:23
@borngraced borngraced mentioned this pull request Feb 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shamardy shamardy shamardy approved these changes

@artemii235 artemii235 artemii235 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@borngraced @artemii235 @shamardy