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

Sapling address encodings #3326

Merged
merged 9 commits into from Jun 19, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Jun 7, 2018

This PR enables Sapling keys and addresses to be passed in anywhere Sprout keys
and addresses are used. Doing so will cause crashes until those places are updated
with Sapling support.

Includes code cherry-picked from the following upstream PRs:

Closes #3058.

str4d and others added some commits Apr 26, 2018

ConvertBits() - convert from one power-of-2 number base to another.
Function extracted from upstream:
  PR bitcoin/bitcoin#11167
  Commit c091b99379b97cb314c9fa123beabdbc324cf7a4
Fix bech32::Encode() error handling
Previously, an input with invalid characters would result in out-of-bounds
reads, potentially exposing up to 224 bytes of memory following the location of
the CHARSET constant. This commit fixes the function to return an empty string,
which is what was originally documented as happening.

@str4d str4d added this to the v2.0.0 milestone Jun 7, 2018

@str4d str4d requested a review from daira Jun 7, 2018

@str4d str4d added this to In Review in Zcashd Team Jun 7, 2018

@@ -153,6 +153,11 @@ class CMainParams : public CChainParams {
// guarantees the first 2 characters, when base58 encoded, are "SK"
base58Prefixes[ZCSPENDING_KEY] = {0xAB,0x36};

bech32HRPs[SAPLING_PAYMENT_ADDRESS] = "zs";

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Use "sapling" instead of "s" for these strings, to be consistent with testnet and regtest?

This comment has been minimized.

@str4d

str4d Jun 12, 2018

Contributor

These are the prefixes of the address encodings. The mainnet one is a) intentionally short, in line with the zc prefix for Sprout addresses, and b) is in the Sapling specification, so we aren't going to change it at this point.

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

Also, that would push it over 80 characters which is a significant usability threshold for cut+pasting from terminals or email.

@@ -133,4 +133,28 @@ bool TimingResistantEqual(const T& a, const T& b)
*/
bool ParseFixedPoint(const std::string &val, int decimals, int64_t *amount_out);

/** Convert from one power-of-2 number base to another. */
template<int frombits, int tobits, bool pad, typename O, typename I>

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Would be nice to have a comment showing a real world example of a value being converted to understand what this function is supposed to do.

This comment has been minimized.

@daira

daira Jun 19, 2018

Contributor

This still needs a comment, at least explaining endianness and the meaning of the pad argument.

std::string operator()(const libzcash::InvalidEncoding& no) const { return {}; }
};

const size_t ConvertedSaplingPaymentAddressSize = ((32 + 11) * 8 + 4) / 5;

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Is this size calculation documented somewhere? If so, please add a comment and link to it, or explain the computation here.

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

Consider having a constexpr function to compute the length.

// ConvertBits requires unsigned char, but CDataStream uses char
std::vector<unsigned char> seraddr(ss.begin(), ss.end());
std::vector<unsigned char> data;
data.reserve((seraddr.size() * 8 + 4) / 5);

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Add comment explaining size computation.

auto sk = SaplingSpendingKey::random();
{
std::string sk_string = EncodeSpendingKey(sk);
BOOST_CHECK(sk_string.compare(0, 24, "secret-spending-key-main") == 0);

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Do we have a constant we can use for "secret-spending-key-main" to avoid typos?

This comment has been minimized.

@str4d

str4d Jun 12, 2018

Contributor

It's defined in the chain parameters; I could use Params() to pull them in here.

auto addr = sk.default_address();

std::string addr_string = EncodePaymentAddress(*addr);
BOOST_CHECK(addr_string.compare(0, 2, "zs") == 0);

This comment has been minimized.

@bitcartel

bitcartel Jun 11, 2018

Contributor

Do we have a constant we can use for "zs" to avoid typos?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 11, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 11, 2018

⌛️ Trying commit bec3e62 with merge 771aee5...

zkbot added a commit that referenced this pull request Jun 11, 2018

Auto merge of #3326 - str4d:3058-sapling-addresses, r=<try>
Sapling address encodings

This PR enables Sapling keys and addresses to be passed in anywhere Sprout keys
and addresses are used. Doing so will cause crashes until those places are updated
with Sapling support.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#11167
- bitcoin/bitcoin#11630

Closes #3058.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 11, 2018

💔 Test failed - pr-try

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 11, 2018

@str4d Reviewed, minor comments. Build test failed due to transient failure. Otherwise looks good.

str4d added some commits Jun 12, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 12, 2018

Addressed @bitcartel's comments.

@daira
Copy link
Contributor

daira left a comment

Partial review.

@@ -153,6 +153,11 @@ class CMainParams : public CChainParams {
// guarantees the first 2 characters, when base58 encoded, are "SK"
base58Prefixes[ZCSPENDING_KEY] = {0xAB,0x36};

bech32HRPs[SAPLING_PAYMENT_ADDRESS] = "zs";

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

Also, that would push it over 80 characters which is a significant usability threshold for cut+pasting from terminals or email.

@@ -150,6 +150,9 @@ std::string Encode(const std::string& hrp, const data& values) {
std::string ret = hrp + '1';
ret.reserve(ret.size() + combined.size());
for (auto c : combined) {
if (c >= 32) {
return "";
}

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

I'm unconvinced that this should be returning "" as an error indicator. It makes more sense for it to be a precondition that values does not contain out-of-range values; failure of the precondition would then be reported via an assertion failure or exception.

This comment has been minimized.

@str4d

str4d Jun 19, 2018

Contributor

If we change bech32::Encode to not return "" on encoding error, then we should also change bech32::Decode to not use an empty hrp to indicate a decoding error, in order to be consistent. That would make the decode logic a bit more complex, as we'd need to separately handle the error case from the wrong-HRP case (whereas currently they are handled together). But would also be technically clearer, so I'm ambivalent.

This comment has been minimized.

@daira

daira Jun 19, 2018

Contributor

I guess it's fine, but it should certainly be better-documented (here and for Decode).

std::string operator()(const libzcash::InvalidEncoding& no) const { return {}; }
};

const size_t ConvertedSaplingPaymentAddressSize = ((32 + 11) * 8 + 4) / 5;

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

Consider having a constexpr function to compute the length.

memory_cleanse(data.data(), data.size());
return ret;
}
}

This comment has been minimized.

@daira

daira Jun 12, 2018

Contributor

This is almost-duplicate code to above, and it will only get worse when viewing keys are implemented. Consider factoring out some of the duplication.

@str4d str4d moved this from In Review to In Progress in Zcashd Team Jun 15, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

Refactoring of code in key_io.cpp would be good to achieve in this PR but could be recorded as a new issue if it helps unblock PRs dependent upon this one.

@str4d str4d moved this from In Progress to In Review in Zcashd Team Jun 19, 2018

@daira

daira approved these changes Jun 19, 2018

Copy link
Contributor

daira left a comment

utACK

BOOST_CHECK_EQUAL(output.size(), 32);
BOOST_CHECK(input == uint256(output));
}
}

This comment has been minimized.

@daira

daira Jun 19, 2018

Contributor

These only test round-tripping and length, so they don't test endianness for example.

@@ -133,4 +133,28 @@ bool TimingResistantEqual(const T& a, const T& b)
*/
bool ParseFixedPoint(const std::string &val, int decimals, int64_t *amount_out);

/** Convert from one power-of-2 number base to another. */
template<int frombits, int tobits, bool pad, typename O, typename I>

This comment has been minimized.

@daira

daira Jun 19, 2018

Contributor

This still needs a comment, at least explaining endianness and the meaning of the pad argument.

@@ -150,6 +150,9 @@ std::string Encode(const std::string& hrp, const data& values) {
std::string ret = hrp + '1';
ret.reserve(ret.size() + combined.size());
for (auto c : combined) {
if (c >= 32) {
return "";
}

This comment has been minimized.

@daira

daira Jun 19, 2018

Contributor

I guess it's fine, but it should certainly be better-documented (here and for Decode).

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Jun 19, 2018

My comments are non-blocking, but it would be good to address them either in this PR or a follow-up.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 19, 2018

Opened #3342, #3343, and #3344 for @daira's comments.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

📌 Commit 69aa0d8 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

⌛️ Testing commit 69aa0d8 with merge 2ebde58...

zkbot added a commit that referenced this pull request Jun 19, 2018

Auto merge of #3326 - str4d:3058-sapling-addresses, r=str4d
Sapling address encodings

This PR enables Sapling keys and addresses to be passed in anywhere Sprout keys
and addresses are used. Doing so will cause crashes until those places are updated
with Sapling support.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#11167
  - Only the `ConvertBits()` function.
- bitcoin/bitcoin#11630

Closes #3058.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 2ebde58 to master...

@zkbot zkbot merged commit 69aa0d8 into zcash:master Jun 19, 2018

1 check passed

homu Test successful
Details

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

@str4d str4d deleted the str4d:3058-sapling-addresses branch Jun 19, 2018

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