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

View Wallet function (rewind_hash and scan rewind_hash) #630

Closed
wants to merge 0 commits into from

Conversation

deevope
Copy link
Contributor

@deevope deevope commented Dec 11, 2021

Features (api&cmdline):

  • get the rewind_hash of the wallet
  • scan of the rewind_hash of a third-party wallet

Goal:
Ensure the transparency (spending, receiving, balance etc) of a wallet that receive grin as donations.
Allow everyone to scan the rewind_hash of the wallet shared publicly.

Cmd usage:
get_rewind_hash: grin-wallet rewind_hash
scan_rewind_hash: grin-wallet scan_rewind_hash 2c95b24de492395934a8a345440ed0ebbe67ee0025b348712a927a91c7fe58e9

@phyro
Copy link
Member

phyro commented Dec 11, 2021

This feature is not only interesting for transparency/auditability but might also be cool to check if you still hold the funds you think you do and how much you hold without needing to have a wallet set up.

P.S. it seems like some tests are failing

@phyro phyro requested a review from yeastplume December 11, 2021 14:24
@@ -75,6 +77,29 @@ where
w.set_parent_key_id_by_name(label)
}

/// Retrieve the slatepack address for the current parent key at
/// the given index
pub fn get_rewind_hash<'a, L, C, K>(
Copy link
Member

@phyro phyro Dec 11, 2021

Choose a reason for hiding this comment

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

I'm wondering whether we should reuse the code from here https://github.com/mimblewimble/grin/blob/master/keychain/src/view_key.rs#L97-L100 to avoid having duplicate computations like this. It's not an issue now, but if/when someone updates one part of the code, it would need to know there exists another computation of the same thing in this other repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Let's do it directly then. I PR this on the node code mimblewimble/grin#3674 in order to use it there.

Choose a reason for hiding this comment

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

Error

let (commit, proof, is_coinbase, height, mmr_index) = output;
let rewind_hash = from_hex(res.rewind_hash.as_str())
.map_err(|e| ErrorKind::GenericError(format!("Unable to create nonce: {}", e)))?;
let rewind_nonce = blake2b(32, &commit.0, &rewind_hash);
Copy link
Member

Choose a reason for hiding this comment

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

#105 describes the hash as rewind_nonce = H(H(pub_root_key), commit) while you have in the code rewind_nonce = H(commit, H(pub_root_key)). I have yet to find whether the implementation actually switches these two around. If you happen to know where I can find this, let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly as of right now which one is true. I'm gonna look at the blake rfc more in depth. Will keep you updated.

FYI, I've just looked to the following function https://github.com/mimblewimble/grin/blob/master/core/src/libtx/proof.rs#L360-L364

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Alright, it seems that the implementation has switched the arguments compared to #105 . You'd also not be able to scan any outputs if it was wrong 👍

);
let result = api.scan_rewind_hash(rewind_hash, Some(start_height));
let deci_sec = time::Duration::from_millis(100);
thread::sleep(deci_sec);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious as to why that's needed; (not a big deal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of the "updater thread" being too slow.

When running grin-wallet rewind_hash:

  • With the sleep():
20211212 09:29:34.150 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning - 99% complete
20211212 09:29:34.174 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning - 99% complete
20211212 09:29:34.175 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning Complete
**20211212 09:29:34.288 WARN grin_wallet_controller::command - View wallet check complete**
  • Without:
20211212 09:29:34.150 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning - 99% complete
**20211212 09:29:34.288 WARN grin_wallet_controller::command - View wallet check complete**
20211212 09:29:34.174 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning - 99% complete
20211212 09:29:34.175 WARN grin_wallet_libwallet::api_impl::owner_updater - Scanning Complete

@yeastplume
Copy link
Member

I think this is brilliant, thank you for doing this! It's the most comprehensive wallet PR we've seen in a while. :D

Just a note for anyone watching, this PR just provides an API for the post 2.0.0 BP rewind scheme outlined and decided upon prior to the 2.0.0 hardfork here: #105. This just covers public view keys and obviously won't work with any outputs created prior to 2.0.0.

If you'd like to just update the rewind hash creation fn (I've merged mimblewimble/grin#3674), I'm happy to merge this unless someone has any objection over the next couple of days.

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