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

feat(zcoin): allow ARRR to sync using a start date #1922

Merged
merged 46 commits into from
Sep 7, 2023

Conversation

borngraced
Copy link
Member

@borngraced borngraced commented Jul 19, 2023

Improve ARRR synchronization based on a user-selected date. This feature will enable users to specify a specific date as the starting point for synchronization as a substitute for the checkpoint block from config or syncing from the first block.

Tasks

  • Added LightWalletSyncParams.date, 'earliestandheight` param for synchronising blocks based on user-selected date, earliest or height for light client

#1900

@borngraced borngraced self-assigned this Jul 19, 2023
@borngraced borngraced added the in progress Changes will be made from the author label Jul 19, 2023
@borngraced borngraced marked this pull request as draft July 19, 2023 09:31
@borngraced borngraced marked this pull request as ready for review July 19, 2023 17:42
@borngraced borngraced changed the title Implement ARRR Sync Starting Block Selection(Date and height) feat(zcoin): implement ARRR Sync Starting Block Selection(Date and height) Jul 19, 2023
@shamardy shamardy removed the in progress Changes will be made from the author label Jul 20, 2023
@shamardy shamardy changed the title feat(zcoin): implement ARRR Sync Starting Block Selection(Date and height) feat(zcoin): allow ARRR to sync using a start date Jul 20, 2023
@shamardy
Copy link
Collaborator

@borngraced please fix wasm clippy warnings and merge to latest dev to pass the adex-cli tests.

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.

Thanks a lot for the fast PR @borngraced! First review iteration, I believe there will be 2 more review iterations after this :)

mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin/storage/walletdb/mod.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs Outdated Show resolved Hide resolved
Signed-off-by: borngraced <[email protected]>
Signed-off-by: borngraced <[email protected]>
… calculate_starting_height_from_date and WalletDbShared init

Signed-off-by: borngraced <[email protected]>
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.

Thanks for the fixes! Next review iteration!

Can you please add a test to test zcoin initialization using date, height or default behaviour. All zcoin tests using light client are ignored, you can try default behaviour for this test and remove the ignore flag

No need to use date or height, you just need to remove checkpoint block from pirate configuration

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
mm2src/coins/z_coin.rs Outdated Show resolved Hide resolved
},
}
},
ZcoinRpcMode::Native => zcoin_builder.protocol_info.check_point_block.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Native should be the same as light client as discussed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be done in another PR as we agreed. Please don't forget to open an issue for it.

@shamardy shamardy added in progress Changes will be made from the author and removed under review labels Jul 26, 2023
@borngraced
Copy link
Member Author

borngraced commented Aug 25, 2023

Ran a couple hundred activation sequence variations in CLI, no problems with activation using expected inputs. There are a few cases where it ignores bad input, not sure if this is a concern but just in case:

  • Handles more than one sync_params value (unsure of hierarchy, but no error)
  • No error on negative value for date, starts from beginning
  • 'sync_params': 'bad_key' does not raise an error, scans from beginning

Yes, @naezith and I concluded on using the earliest date incase bad date/height was provided

smk762
smk762 previously approved these changes Aug 25, 2023
Copy link

@smk762 smk762 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

shamardy
shamardy previously approved these changes Aug 29, 2023
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.

@Alrighttt @ca333 @DeckerSU To help with security review, I marked the places where I think it's required. You can ofcourse check the whole PR for other things :)

Comment on lines +146 to +155
pub(crate) async fn get_earliest_block(&self) -> Result<u32, ZcashClientError> {
Ok(query_single_row(
&self.db.lock().unwrap(),
"SELECT MIN(height) from compactblocks",
[],
|row| row.get::<_, Option<u32>>(0),
)?
.flatten()
.unwrap_or(0))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

sec-review

run: |
Invoke-WebRequest -Uri https://github.com/KomodoPlatform/komodo/raw/d456be35acd1f8584e1e4f971aea27bd0644d5c5/zcutil/wget64.exe -OutFile \wget64.exe
Invoke-WebRequest -Uri https://raw.githubusercontent.com/KomodoPlatform/komodo/master/zcutil/fetch-params-alt.bat -OutFile \cmd.bat && \cmd.bat
cargo test --test 'mm2_tests_main' --no-fail-fast
Copy link
Collaborator

Choose a reason for hiding this comment

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

sec-review

Comment on lines +120 to +122
run: |
wget -O - https://raw.githubusercontent.com/KomodoPlatform/komodo/master/zcutil/fetch-params-alt.sh | bash
cargo test --test 'mm2_tests_main' --no-fail-fast
Copy link
Collaborator

Choose a reason for hiding this comment

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

sec-review

ca333
ca333 previously approved these changes Sep 1, 2023
Copy link

@ca333 ca333 left a comment

Choose a reason for hiding this comment

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

secure code reviewed

@shamardy
Copy link
Collaborator

shamardy commented Sep 4, 2023

@borngraced can you please resolve the git conflicts in this PR?

# Conflicts:
#	mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs
#	mm2src/coins/z_coin.rs
#	mm2src/coins/z_coin/storage/walletdb/mod.rs
#	mm2src/mm2_main/tests/integration_tests_common/mod.rs
#	mm2src/mm2_main/tests/mm2_tests/z_coin_tests.rs
#	mm2src/mm2_test_helpers/src/for_tests.rs
Signed-off-by: borngraced <[email protected]>
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Good job!
As we are going to prepare our code base for missing_docs lint in CI, its necessary to add doc comments for all pub items, bcz lint will deny pub items without them.
For the current PR please do it at least for items, which were added or changed.

mm2src/mm2_test_helpers/src/structs.rs Show resolved Hide resolved
mm2src/mm2_test_helpers/src/structs.rs Show resolved Hide resolved
mm2src/mm2_test_helpers/src/structs.rs Show resolved Hide resolved
mm2src/coins/z_coin/z_coin_errors.rs Show resolved Hide resolved
mm2src/coins/z_coin.rs Show resolved Hide resolved
mm2src/coins/z_coin.rs Show resolved Hide resolved
mm2src/coins/z_coin.rs Show resolved Hide resolved
@laruh laruh mentioned this pull request Sep 5, 2023
8 tasks
Signed-off-by: borngraced <[email protected]>
@smk762 smk762 requested review from smk762 and kivqa September 6, 2023 10:53
smk762
smk762 previously approved these changes Sep 6, 2023
Signed-off-by: borngraced <[email protected]>
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Thank you for your work!
I have just a note for the future. Perhaps you have already discussed this in conversations above, if so, what decision did you make?

async fn get_block_height(&mut self) -> Result<u64, MmError<UpdateBlocksCacheErr>>;

/// Asynchronously retrieve the tree state for a specific block height from the Zcoin network.
#[cfg(not(target_arch = "wasm32"))]
async fn get_tree_state(&mut self, height: u64) -> Result<TreeState, MmError<UpdateBlocksCacheErr>>;
Copy link
Member

Choose a reason for hiding this comment

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

NativeClient doesnt support grpc client (i mean you return TreeState here, which is protobuf message), but as get_tree_state is in todo (in NativeClient) and we are going to use this functionality on LightRpcClient for now, I think its fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

plan to implement a get_tree_state, LightdInfo rpc methods for native client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants