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

3604 fix sapling default memo - 0xF6 plus zeros #3605

Merged
merged 2 commits into from Oct 28, 2018

Conversation

@LarryRuane
Copy link
Contributor

LarryRuane commented Oct 15, 2018

Closes #3604. For Sapling outputs, set the default memo to no_memo instead of all zeros. (See section 5.5 of the Sapling protocol specification.)

@LarryRuane LarryRuane force-pushed the LarryRuane:3604-memo-zeros branch from 5b5d701 to f2844b1 Oct 15, 2018

@LarryRuane LarryRuane requested review from daira and str4d Oct 15, 2018

@LarryRuane LarryRuane changed the title 3604 memo zeros 3604 fix sapling default memo - 0xF6 plus zeros Oct 15, 2018

@LarryRuane LarryRuane force-pushed the LarryRuane:3604-memo-zeros branch from f2844b1 to 4bc6d1b Oct 15, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 18, 2018

@LarryRuane Maybe in one of the Sapling tests, where we list the z transaction, verify the memo field begins with F6, whereas it would currently fail with 00.

@Eirik0 Eirik0 added this to In Review in Zcashd Team Oct 22, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Oct 22, 2018

@bitcartel, please elaborate... Which Sapling tests? Are these rpc tests, boost tests, or gtests? The listtransactions RPC doesn't show the memo, although maybe it could and should?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 22, 2018

https://github.com/zcash/zcash/blob/master/qa/rpc-tests/wallet_listreceived.py#L82 is the test that needs updating. There's a bug in the referenced line (it should be checking r[1]['memo'] against my_memo, otherwise it's just doubling-up a previous check), but it isn't the bug I thought it was a moment ago (in a deleted comment I thought it was a bug that, when fixed, would test this change).

@LarryRuane LarryRuane force-pushed the LarryRuane:3604-memo-zeros branch from 4bc6d1b to 245db9c Oct 24, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Oct 24, 2018

@str4d, I pushed a fix for the test (it fails without the zcashd fix, succeeds with it), thanks for noticing that, good catch! I'm actually planning to replace that test (unify it with another test) in #3553 but it's good to keep this PR self-consistent.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 24, 2018

ACK

@Eirik0

Eirik0 approved these changes Oct 24, 2018

@daira

daira approved these changes Oct 25, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov)

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 28, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 28, 2018

📌 Commit 245db9c has been approved by daira

zkbot added a commit that referenced this pull request Oct 28, 2018

Auto merge of #3605 - LarryRuane:3604-memo-zeros, r=daira
3604 fix sapling default memo - 0xF6 plus zeros

Closes #3604. For Sapling outputs, set the default memo to `no_memo` instead of all zeros. (See section 5.5 of the Sapling protocol specification.)
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 28, 2018

⌛️ Testing commit 245db9c with merge 97fccdb...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 28, 2018

☀️ Test successful - pr-merge
Approved by: daira
Pushing 97fccdb to master...

@zkbot zkbot merged commit 245db9c into zcash:master Oct 28, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Oct 28, 2018

@zkbot zkbot referenced this pull request Oct 28, 2018

Open

3553 listnotes rpc test #3569

@str4d str4d added this to the v2.0.2 milestone Nov 16, 2018

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