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

Disable Sapling features on mainnet #3458

Merged
merged 3 commits into from Aug 14, 2018
Merged

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Aug 13, 2018

Also places them behind an experimental features flag on testnet.

@str4d str4d added A-rpc-interface Area: RPC interface NU1-sapling Network upgrade: Sapling-specific tasks labels Aug 13, 2018
@str4d str4d added this to the v2.0.0 milestone Aug 13, 2018
@str4d str4d added this to In Review in Arborist Team Aug 13, 2018
@str4d
Copy link
Contributor Author

str4d commented Aug 13, 2018

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Aug 13, 2018

⌛ Trying commit 554e00e with merge 511b1a085349f0bb786dc40644f74c30a0daebd3...

@zkbot
Copy link
Contributor

zkbot commented Aug 13, 2018

☀️ Test successful - pr-try
State: approved= try=True

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK. Do we need a test of the -developersapling flag? One way to do that would be to reenable the EncodeAndDecodeSapling test but have it set the flag.

for (size_t i = 0; i < 1000; i++) {
auto sk = SaplingSpendingKey::random();
{
std::string sk_string = EncodeSpendingKey(sk);
BOOST_CHECK(sk_string.compare(0, 24, Params().Bech32HRP(CChainParams::SAPLING_SPENDING_KEY)) == 0);
BOOST_CHECK(sk_string.compare(0, 27, Params().Bech32HRP(CChainParams::SAPLING_SPENDING_KEY)) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 24 -> 27 ? For regtest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is the length of the HRP, and C++'s std::string::compare takes the data range over which to compare as parameters.

@@ -240,7 +242,7 @@ BOOST_AUTO_TEST_CASE(zs_address_test)
auto addr = sk.default_address();

std::string addr_string = EncodePaymentAddress(addr);
BOOST_CHECK(addr_string.compare(0, 2, Params().Bech32HRP(CChainParams::SAPLING_PAYMENT_ADDRESS)) == 0);
BOOST_CHECK(addr_string.compare(0, 15, Params().Bech32HRP(CChainParams::SAPLING_PAYMENT_ADDRESS)) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 2 -> 15 ? For regtest? Would be good to define a well-named constant for these magic values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above. Not magic per-se, could have just used len() or whatever on the output of Bech32HRP().

{
std::string sk_string = EncodeSpendingKey(sk);
EXPECT_EQ(
sk_string.substr(0, 24),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 27, based on changes to key_tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a test explicitly of the mainnet parameters, so it uses the mainnet lengths.


std::string addr_string = EncodePaymentAddress(addr);
EXPECT_EQ(
addr_string.substr(0, 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, why not 15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.


#include <gtest/gtest.h>

TEST(Keys, DISABLED_EncodeAndDecodeSapling)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this test works, why disable it? Should be ok to test against mainnet key encodings, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test works until Sapling support in Decode* is disabled. Marking this as a disabled test causes it to be run by the expected-fail CI builder, so we ensure that it currently fails, and detect that it is no longer failing once the changes to Decode* are reverted, if this test is not also reverted.

BOOST_CHECK_EQUAL(b, true);
BOOST_CHECK_EQUAL(find_value(resultObj, "type").get_str(), "sapling");
b = find_value(resultObj, "ismine").get_bool();
// TODO: Revert when we re-enable Sapling addresses on mainnet
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear what gets reverted and developer will have to check commit. How about keeping old test suitably commented between /** and */ with instructions? Maybe use in conjunction with #if 1 , #if 0 blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this PR is designed, all we need to do is a git revert COMMIT_ID for the last commit in this PR (that disabled Sapling support), and everything will be correctly reverted.

@@ -286,7 +286,11 @@ libzcash::PaymentAddress DecodePaymentAddress(const std::string& str)
}
data.clear();
auto bech = bech32::Decode(str);
if (bech.first == Params().Bech32HRP(CChainParams::SAPLING_PAYMENT_ADDRESS) &&
bool allowSapling = Params().NetworkIDString() == "regtest" || (
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a global function for this since repeated in several places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not worth the overhead, as we'll only be using this for a few weeks, and then will strip it out completely.

@@ -3133,9 +3133,13 @@ UniValue z_getnewaddress(const UniValue& params, bool fHelp)
addrType = params[0].get_str();
}

bool allowSapling = Params().NetworkIDString() == "regtest" || (
Copy link
Contributor

Choose a reason for hiding this comment

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

What about z_importkey and z_exportkey? Do they need to be gated or not?

Copy link
Contributor Author

@str4d str4d Aug 13, 2018

Choose a reason for hiding this comment

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

z_importkey is gated because DecodePaymentAddress has been modified to remove Sapling support on mainnet and gate it on testnet. z_exportkey is handled because we have done the same removal/gating to z_getnewaddress sapling, and there is no other way to add a Sapling key to the wallet.

Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

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

utACK. One suggestion: modify the help messages of the relevant z_* commands which take Saplng parameters so that users know they only work in regtest mode or testnet with the appropriate flags.

@ebfull
Copy link
Contributor

ebfull commented Aug 14, 2018

I will be r+'ing this in 12 hours to avoid competition with the activation height PR.

@ebfull
Copy link
Contributor

ebfull commented Aug 14, 2018

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Aug 14, 2018

📌 Commit 554e00e has been approved by ebfull

zkbot added a commit that referenced this pull request Aug 14, 2018
Disable Sapling features on mainnet

Also places them behind an experimental features flag on testnet.
@zkbot
Copy link
Contributor

zkbot commented Aug 14, 2018

⌛ Testing commit 554e00e with merge b004f25...

@zkbot
Copy link
Contributor

zkbot commented Aug 14, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing b004f25 to master...

@zkbot zkbot merged commit 554e00e into zcash:master Aug 14, 2018
Arborist Team automation moved this from In Review to Released (Merged in Master) Aug 14, 2018
@str4d str4d deleted the temp-sapling-disable branch August 15, 2018 08:15
zkbot added a commit that referenced this pull request Sep 22, 2018
Enable Sapling features on mainnet

Reverts the last commit from #3458.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface NU1-sapling Network upgrade: Sapling-specific tasks
Projects
Arborist Team
  
Released (Merged in Master)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants