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

Implement incoming viewing keys #2143

Merged
merged 12 commits into from Dec 20, 2017

Conversation

@str4d

str4d commented Mar 2, 2017

Closes #1997.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Mar 2, 2017

Currently the unit test of CWalletDD::WriteViewingKey() fails. @bitcartel you wrote the original versions of those tests for spending keys; could you have a look?

str4d commented Mar 2, 2017

Currently the unit test of CWalletDD::WriteViewingKey() fails. @bitcartel you wrote the original versions of those tests for spending keys; could you have a look?

@daira

Looks pretty good so far 👍

Show outdated Hide outdated src/wallet/rpcwallet.cpp Outdated
@@ -471,6 +483,19 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
// so set the wallet birthday to the beginning of time.
pwallet->nTimeFirstKey = 1;
}
else if (strType == "vkey")

This comment has been minimized.

@daira

daira Mar 6, 2017

vkey also needs to be added to the IsKeyType function, I think.

@daira

daira Mar 6, 2017

vkey also needs to be added to the IsKeyType function, I think.

This comment has been minimized.

@str4d

str4d Mar 6, 2017

I wasn't sure about that; the watch addresses aren't there. IIUC the IsKeyType function is used to try and protect spending keys in the wallet in case of detected corruption.

@str4d

str4d Mar 6, 2017

I wasn't sure about that; the watch addresses aren't there. IIUC the IsKeyType function is used to try and protect spending keys in the wallet in case of detected corruption.

This comment has been minimized.

@daira

daira Dec 16, 2017

Right, but viewing keys should also be so protected.

@daira

daira Dec 16, 2017

Right, but viewing keys should also be so protected.

This comment has been minimized.

@bitcartel

bitcartel Dec 20, 2017

This is resolved now.

@bitcartel

bitcartel Dec 20, 2017

This is resolved now.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Mar 6, 2017

Sorry, didn't mean to approve this since it's still a WIP, and tests don't pass.

daira commented Mar 6, 2017

Sorry, didn't mean to approve this since it's still a WIP, and tests don't pass.

@zmanian

This comment has been minimized.

Show comment
Hide comment
@zmanian

zmanian Mar 7, 2017

Is this worth testing with the viewing key generator in zcash-mini yet? Happy to do that when it is.

zmanian commented Mar 7, 2017

Is this worth testing with the viewing key generator in zcash-mini yet? Happy to do that when it is.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Mar 7, 2017

@zmanian not yet, but I'll ping you when it is ☺️

str4d commented Mar 7, 2017

@zmanian not yet, but I'll ping you when it is ☺️

@daira daira dismissed their stale review Mar 14, 2017

Didn't intend to approve this yet.

@nathan-at-least nathan-at-least changed the title from [WIP] Implement viewing keys to Implement viewing keys Apr 5, 2017

@daira daira requested review from bitcartel and ebfull and removed request for bitcartel Apr 10, 2017

@daira daira changed the title from Implement viewing keys to Implement incoming viewing keys Apr 20, 2017

@str4d str4d removed the work-in-progress label Apr 20, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Apr 20, 2017

I believe this is feature-complete now, and therefore ready for review.

str4d commented Apr 20, 2017

I believe this is feature-complete now, and therefore ready for review.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Jun 16, 2017

☔️ The latest upstream changes (presumably #2440) made this pull request unmergeable. Please resolve the merge conflicts.

zkbot commented Jun 16, 2017

☔️ The latest upstream changes (presumably #2440) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Aug 14, 2017

Rebased onto master to fix merge conflicts and a small bug (see the second diff section of 2945bd5).

str4d commented Aug 14, 2017

Rebased onto master to fix merge conflicts and a small bug (see the second diff section of 2945bd5).

@nathan-at-least nathan-at-least added this to Proposed in Release planning Aug 15, 2017

@nathan-at-least nathan-at-least moved this from Proposed to Misc in Release planning Aug 15, 2017

@str4d str4d self-assigned this Aug 15, 2017

@str4d str4d moved this from Misc to 1.0.13: Experimental Features in Release planning Aug 16, 2017

@str4d str4d moved this from 1.0.13: Experimental Features to 1.0.14: Experimental Features in Release planning Aug 21, 2017

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 14, 2017

☔️ The latest upstream changes (presumably #2159) made this pull request unmergeable. Please resolve the merge conflicts.

zkbot commented Nov 14, 2017

☔️ The latest upstream changes (presumably #2159) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 21, 2017

Rebased on master to fix merge conflicts.

str4d commented Nov 21, 2017

Rebased on master to fix merge conflicts.

@tromer

This comment has been minimized.

Show comment
Hide comment
@tromer

tromer Nov 21, 2017

Can we add a prefix to serialized incoming viewing keys, to distinguish them from other hex blobs, a la #2746? I suggest zivk: (Zcash incoming view key).

The RPC names z_importviewingkey and z_exportviewingkey will create a problem when outgoing viewing keys are added. Rename to z_importincomingviewingkey and z_exportincomingviewingkey?

The identifiers in the code should be similarly specific.

As a matter of style, I think "view key" sounds as good as "viewing key" and is shorter.

tromer commented Nov 21, 2017

Can we add a prefix to serialized incoming viewing keys, to distinguish them from other hex blobs, a la #2746? I suggest zivk: (Zcash incoming view key).

The RPC names z_importviewingkey and z_exportviewingkey will create a problem when outgoing viewing keys are added. Rename to z_importincomingviewingkey and z_exportincomingviewingkey?

The identifiers in the code should be similarly specific.

As a matter of style, I think "view key" sounds as good as "viewing key" and is shorter.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Dec 20, 2017

Note that this PR has been included in the Github project "Experimental Features..."

bitcartel commented Dec 20, 2017

Note that this PR has been included in the Github project "Experimental Features..."

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Dec 20, 2017

ACK (Github is not showing me the green button to approve)

bitcartel commented Dec 20, 2017

ACK (Github is not showing me the green button to approve)

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

The spend issue might confuse users. The release notes should describe the use-case for incoming viewing keys and why they might want them e.g. opening comment on #1997.

Yep. I will add a commit with release notes before merging this.

Note that this PR has been included in the Github project "Experimental Features..."

Oh, sorry! That was because this PR was a prerequisite for #2542, which will be an experimental feature, but never made it into that column.

str4d commented Dec 20, 2017

The spend issue might confuse users. The release notes should describe the use-case for incoming viewing keys and why they might want them e.g. opening comment on #1997.

Yep. I will add a commit with release notes before merging this.

Note that this PR has been included in the Github project "Experimental Features..."

Oh, sorry! That was because this PR was a prerequisite for #2542, which will be an experimental feature, but never made it into that column.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel commented Dec 20, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

📌 Commit 5221220 has been approved by bitcartel

zkbot commented Dec 20, 2017

📌 Commit 5221220 has been approved by bitcartel

zkbot added a commit that referenced this pull request Dec 20, 2017

Auto merge of #2143 - str4d:1997-viewing-keys, r=bitcartel
Implement incoming viewing keys

Closes #1997.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

⌛️ Testing commit 5221220 with merge b9fd173...

zkbot commented Dec 20, 2017

⌛️ Testing commit 5221220 with merge b9fd173...

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Dec 20, 2017

@str4d build hanging or just takes longer these days?

bitcartel commented Dec 20, 2017

@str4d build hanging or just takes longer these days?

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

str4d commented Dec 20, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

str4d commented Dec 20, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

str4d commented Dec 20, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

str4d commented Dec 20, 2017

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

📌 Commit 5221220 has been approved by str4d

zkbot commented Dec 20, 2017

📌 Commit 5221220 has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

⌛️ Testing commit 5221220 with merge 75b08dc...

zkbot commented Dec 20, 2017

⌛️ Testing commit 5221220 with merge 75b08dc...

zkbot added a commit that referenced this pull request Dec 20, 2017

Auto merge of #2143 - str4d:1997-viewing-keys, r=str4d
Implement incoming viewing keys

Closes #1997.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

💔 Test failed - pr-merge

zkbot commented Dec 20, 2017

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 20, 2017

str4d commented Dec 20, 2017

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

⌛️ Testing commit 5221220 with merge 1683c2d...

zkbot commented Dec 20, 2017

⌛️ Testing commit 5221220 with merge 1683c2d...

zkbot added a commit that referenced this pull request Dec 20, 2017

Auto merge of #2143 - str4d:1997-viewing-keys, r=str4d
Implement incoming viewing keys

Closes #1997.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Dec 20, 2017

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 1683c2d to master...

zkbot commented Dec 20, 2017

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 1683c2d to master...

@zkbot zkbot merged commit 5221220 into zcash:master Dec 20, 2017

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:1997-viewing-keys branch Dec 20, 2017

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