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.

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

[r2r] Fix ARRR activation. More info to activation statuses. Check point block #1439

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

artemii235
Copy link
Member

@artemii235 artemii235 commented Aug 9, 2022

Light wallet sync process could start from zero block, for which Sapling might be not activated yet. It caused unrecoverable error from lightwalletd with endless activation process on MM2 side.
#1427
KomodoPlatform/komodo-wallet#1775 (comment)

@artemii235 artemii235 changed the title [wip] Fix ARRR activation. More info to activation statuses. Check point block [r2r] Fix ARRR activation. More info to activation statuses. Check point block Aug 9, 2022
shamardy
shamardy previously approved these changes Aug 9, 2022
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! Also one non-blocker comment :)

mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
@artemii235
Copy link
Member Author

@sergeyboyko0791 @ozkanonur Could you please review it too? I assigned P0 to this because it's a hotfix required for ARRR GUI integration release.

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Only one note

})
.is_err()
{
debug!("No one seems interested in SyncStatus");

Choose a reason for hiding this comment

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

Not critical. We could add LogOnError::debug_log_with_msg and use it here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will do 🙂

mm2src/coins/z_coin/z_rpc.rs Outdated Show resolved Hide resolved
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Great fix. Just one note.

let mut from_block = self
.consensus_params
.sapling_activation_height
.max(current_block_in_db as u32 + 1) as u64;
Copy link
Member

Choose a reason for hiding this comment

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

current_block_in_db is already u32. No need to re-cast to same type.

Choose a reason for hiding this comment

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

Hmm, get_latest_block now returns u32, missed it

Copy link
Member Author

Choose a reason for hiding this comment

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

@ozkanonur Done

@artemii235 artemii235 changed the title [r2r] Fix ARRR activation. More info to activation statuses. Check point block [wip] Fix ARRR activation. More info to activation statuses. Check point block Aug 10, 2022
Copy link

@sergeyboyko0791 sergeyboyko0791 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 changed the title [wip] Fix ARRR activation. More info to activation statuses. Check point block [r2r] Fix ARRR activation. More info to activation statuses. Check point block Aug 10, 2022
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

🔥

@artemii235 artemii235 merged commit 0f6c726 into mm2.1 Aug 10, 2022
@artemii235 artemii235 deleted the arrr-activation-hotfix branch August 10, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants