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

Spv proof #1207

Merged
merged 82 commits into from
Mar 23, 2022
Merged

Spv proof #1207

merged 82 commits into from
Mar 23, 2022

Conversation

Milerius
Copy link

@Milerius Milerius commented Feb 8, 2022

  • Please check if the PR fulfills these requirements
  • Tests for the changes have been added (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • spv_validation crate with validation functions for headers and transaction
  • parameters for block header storage/validation (difficulty check, constant difficulty, nb blocks to verify)
  • update toolchain to nightly-2022-02-01
  • implement SQL storage for block headers
  • implement an empty IndexedDB storage for block headers
  • implement a function to retrieve a block header either from network or storage, add to storage/validate headers if required
  • implement a loop/task for downloading last block headers every N minutes (configurable)
  • some cargo clippy fixing due to new toolchain update
  • new dependencies: bitcoin-spv
  • What is the current behavior? (You can also link to an open issue here)
  • No spv validation in validate_payment
  • What is the new behavior (if this is a feature change)?
  • Add an spv validation in validate_payment
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  • No, everything is configurable and ignored if not configured.
  • Other information:

- native client should not return error on validate_spv_proof since there is no verification
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.

Thanks for the fixes. Last comments :)

mm2src/coins/eth/eth_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo.rs Show resolved Hide resolved
@@ -315,6 +320,11 @@ pub trait UtxoCoinBuilderCommonOps {

fn ticker(&self) -> &str;

fn block_headers_storage(&self) -> Option<BlockHeaderStorage> {
let params = json::from_value(self.conf()["block_header_params"].clone()).ok()?;

Choose a reason for hiding this comment

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

Could you please return an error if we couldn't parse block_header_params?

fn block_headers_storage(&self) -> UtxoCoinBuildResult <Option<BlockHeaderStorage>> {
        let params: Option<_> = json::from_value(self.conf()["block_header_params"].clone())?;
        match params {
                Some(params) => BlockHeaderStorage::new_from_ctx(self.ctx().clone(), params),
                None => Ok(None)
        }
}

@artemii235
Copy link
Member

@Milerius Is it ready for the next review iteration?

@Milerius
Copy link
Author

Milerius commented Mar 16, 2022

@Milerius Is it ready for the next review iteration?

I'm currently checking on #1207 (comment) other than that yes.

edit: #1207 (comment) I confirm that we need to keep an Option for the block header storage, every coins doesn't need a storage for block headers, it depends if the option is set or not in the conf file - ready for review.

@Milerius Milerius requested a review from artemii235 March 16, 2022 09:37
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.

Next review iteration.

mm2src/coins/utxo.rs Show resolved Hide resolved
mm2src/coins/utxo.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved
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.

Next review iteration

mm2src/coins/utxo/utxo_block_header_storage.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_builder/utxo_arc_builder.rs Outdated Show resolved Hide resolved
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.

Another comment :)

mm2src/coins/utxo/utxo_block_header_storage.rs Outdated Show resolved Hide resolved
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 minor code improvements proposals.

mm2src/mm2_bitcoin/spv_validation/src/spv_proof.rs Outdated Show resolved Hide resolved
artemii235
artemii235 previously approved these changes Mar 22, 2022
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!

@artemii235
Copy link
Member

@shamardy @sergeyboyko0791 Please check if all your comments are resolved.

shamardy
shamardy previously approved these changes Mar 22, 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.

🔥

@Milerius Milerius dismissed stale reviews from shamardy and artemii235 via 4ff3f4f March 23, 2022 10:13
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!

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

Successfully merging this pull request may close these issues.

4 participants