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

Add Sapling support to z_shieldcoinbase #3518

Merged
merged 4 commits into from Oct 5, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Sep 13, 2018

Part of #3216.

@str4d str4d added this to the v2.0.1 milestone Sep 13, 2018

@str4d str4d requested review from bitcartel , ebfull and Eirik0 Sep 13, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 13, 2018

This PR uses the same FVK strategy as #3516.

@str4d str4d added this to In Review in Zcashd Team Sep 13, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 13, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 13, 2018

⌛️ Trying commit 23f1b17 with merge c9db52b...

zkbot added a commit that referenced this pull request Sep 13, 2018

Auto merge of #3518 - str4d:3216-z_shieldcoinbase, r=<try>
Add Sapling support to z_shieldcoinbase

Part of #3216.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 14, 2018

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

} else if (tChangeAddr) {
// tChangeAddr has already been validated.
assert(AddTransparentOutput(tChangeAddr.value(), change));
} else if (!spends.empty()) {
auto fvk = spends[0].expsk.full_viewing_key();
auto note = spends[0].note;
libzcash::SaplingPaymentAddress changeAddr(note.d, note.pk_d);
AddSaplingOutput(fvk, changeAddr, change, {});

This comment has been minimized.

@Eirik0

Eirik0 Sep 17, 2018

Contributor

Is it incorrect to pass in an empty vector? I ask because it is still possible to do this, for example, we do this in test_transaction_builder.cpp and test_wallet.cpp. I was wondering if it was something that should be enforced in the TransactionBuilder::Build() method.

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

Yeah, this is something I messed up in the initial implementation. We denote an empty memo field with an invalid UTF-8 byte, and then all-zeros afterwards. If the leading byte is a valid UTF-8 character (of which 0 is), then the entire field is to be treated as a UTF-8 string, rather than empty.

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

It's not something to enforce in Build(), because if the user wants the field to be the all-zeroes UTF-8 array, then fine. It's about being consistent as to how we signal "there is no memo field here".


def setup_chain(self):
print("Initializing test directory "+self.options.tmpdir)
initialize_chain_clean(self.options.tmpdir, 4)

def setup_network(self, split=False):
args = ['-regtestprotectcoinbase', '-debug=zrpcunsafe']
args2 = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', "-mempooltxinputlimit=7"]
if self.addr_type != 'sprout':

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

Nit: Why not if self.addr_type == 'sapling'?

Also, this block might deserve a comment. We're removing -mempooltxinputlimit=7 from args2 and its not clear why.

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

If there are future address types, Overwinter and Sapling should also be activated for them.

I suspect that Overwinter should be activated when the address type is "sprout", but that is covered by #2966 I think. (If I'm guessing correctly as to why "-mempooltxinputlimit=7" is no longer needed, that would also apply when Overwinter is activated but Sapling is not.)

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

Looking at the remainder of the test, it definitely seems as though coverage would benefit from running it for all three cases (launch, post-Overwinter, post-Sapling).

// Send the transaction
// TODO: Use CWallet::CommitTransaction instead of sendrawtransaction
auto signedtxn = EncodeHexTx(m_op->tx_);
if (!m_op->testmode) {

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

This whole if/else should be extracted into a method. This can be done later.

This comment has been minimized.

@str4d

str4d Sep 19, 2018

Contributor

This is copied from z_sendmany for now. We should address this as part of #2979, as well as addressing the TODO item.

@@ -3967,6 +3967,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
}

utxoCounter++;
auto scriptPubKey = out.tx->vout[out.i].scriptPubKey;

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

Nit: The scope of this could be reduced.

@@ -3977,7 +3978,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
maxedOutFlag = true;
} else {
estimatedTxSize += increase;
ShieldCoinbaseUTXO utxo = {out.tx->GetHash(), out.i, nValue};
ShieldCoinbaseUTXO utxo = {out.tx->GetHash(), out.i, scriptPubKey, nValue};

This comment has been minimized.

@Eirik0

Eirik0 Sep 18, 2018

Contributor

Why this change?

This comment has been minimized.

@str4d

str4d Sep 19, 2018

Contributor

z_sendmany etc. were written to use signrawtransaction (by marshalling to JSON, calling the RPC, and then unmarshalling), which looks up the scriptPubKey for each input. TransactionBuilder implements transparent input signing directly, so it needs to be given this value. When we refactor to address the TODO about using CWallet::CommitTransaction, we will end up needing scriptPubKey for the non-builder case as well.

@str4d str4d force-pushed the str4d:3216-z_shieldcoinbase branch from 23f1b17 to 80e5116 Sep 18, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

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

@str4d str4d force-pushed the str4d:3216-z_shieldcoinbase branch from 80e5116 to f85ba99 Sep 19, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 19, 2018

Rebased to fix merge conflicts and use strategy from #3516.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 20, 2018

@str4d When letting the user choose "no limit" for the number of utxos to shield, we get bad sigops.

./zcash-cli z_shieldcoinbase t2Vf4wKcJ3ZFtLj4jezUUKkwYR92BLHn5UT ztestsapling19fpdyuq3n63wyfdep74960qxgnxtdu6l3d0qwnqvd4xyp03yylcjhy0y9qy5hvpdz3sz2deceg2 0.0001 0
{
  "remainingUTXOs": 6112,
  "remainingValue": 15280.00000000,
  "shieldingUTXOs": 4994,
  "shieldingValue": 12485.00000000,
  "opid": "opid-509fc46f-b926-4e59-8130-d1fc4cb7b848"
}

./zcash-cli z_getoperationresult
[
  {
    "id": "opid-509fc46f-b926-4e59-8130-d1fc4cb7b848",
    "status": "failed",
    "creation_time": 1537457161,
    "error": {
      "code": -26,
      "message": "64: bad-txns-too-many-sigops"
    },
    "method": "z_shieldcoinbase",
    "params": {
      "fromaddress": "t2Vf4wKcJ3ZFtLj4jezUUKkwYR92BLHn5UT",
      "toaddress": "ztestsapling19fpdyuq3n63wyfdep74960qxgnxtdu6l3d0qwnqvd4xyp03yylcjhy0y9qy5hvpdz3sz2deceg2",
      "fee": 0.00010000
    }
  }
]

Here is a tx shielding 1000 coinbase utxos:
https://explorer.testnet.z.cash/tx/cf4cb56e490f60df27fe286976797b8971d42f568c5c927a5733ebe35e55729c

From trial and error, the most I got to work was 1333 coinbase utxos. That created a transaction which was accepted into the local mempool, but it was never mined.

It seems the transaction was rejected because the default fee of 0.0001 I specified was too low given the size of the transaction, so it was treated as "free", probably like the earlier transaction. In debug.log I see: Reject tx code 66: rate limited free transaction:

Here is a tx shielding 1333 coinbase utxos:
https://explorer.testnet.z.cash/tx/bb52cd2fd2f8518f7eef5948998ec2f8863a5a9392b03d845e42c296957d6d25

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 20, 2018

If this PR doesn't make it in time, then this needs to be implemented for z_shieldcoinbase otherwise users will terminate their nodes when trying to use the RPC.
#3533

import os

cwd = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe())))
execfile(os.path.join(cwd, 'wallet_shieldcoinbase.py'))

This comment has been minimized.

@Eirik0

Eirik0 Sep 22, 2018

Contributor

What are the above lines doing?

This comment has been minimized.

@str4d

str4d Sep 26, 2018

Contributor

The way the RPC test runner works, the various RPC test files don't actually see themselves as being in the same module, so we can't use import. This code finds the file we want in the same directory, and then executes it as if it was code in the same file.

cwd = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe())))
execfile(os.path.join(cwd, 'wallet_shieldcoinbase.py'))

class WalletShieldCoinbaseSprout(WalletShieldCoinbaseTest):

This comment has been minimized.

@Eirik0

Eirik0 Sep 22, 2018

Contributor

This is the sapling test, but the class is named sprout.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 26, 2018

To follow up, the sigops error appears with sprout zaddr too.

2018-09-26 19:22:50 opid-88f5e166-3d35-437e-99fc-e715bf16c140: z_shieldcoinbase initialized (context={"fromaddress":"t2Vf4wKcJ3ZFtLj4jezUUKkwYR92BLHn5UT","toaddress":"ztnQxPkuyWEX2asXwUq85UL6CxEwXPyTgwkB4NmzytqnHPXiuSRP5dvkC3oHVTszjMnE5zncbAd8fxfcqzJWWZx1KDw9eQv","fee":0.00010000})
2018-09-26 19:22:50 opid-88f5e166-3d35-437e-99fc-e715bf16c140: spending 12485.00 to shield 12484.9999 with fee 0.0001
2018-09-26 19:22:50 opid-88f5e166-3d35-437e-99fc-e715bf16c140: creating joinsplit at index 0 (vpub_old=12484.9999, vpub_new=0.00, in[0]=0.00, in[1]=0.00, out[0]=12484.9999, out[1]=0.00)
2018-09-26 19:24:05 ERROR: AcceptToMemoryPool: too many sigops ad7a0683ca1c8ce1a5c69e48442c7c49c83aeefd4852393e743115f455d2d8ff, 14982 > 4000
2018-09-26 19:24:05 opid-88f5e166-3d35-437e-99fc-e715bf16c140: z_shieldcoinbase finished (status=failed, error=64: bad-txns-too-many-sigops)

The 4000 limit mentioned in the log is inherited from upsteam:

consensus/consensus.h:24:static const unsigned int MAX_BLOCK_SIGOPS = 20000;

main.h:66:static const unsigned int MAX_STANDARD_TX_SIGOPS = MAX_BLOCK_SIGOPS/5;
@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 27, 2018

Increase the default limit of utxos to shield?
Increase the max block sigops but this is a consensus rule.?
Increase the max tx sigops, which is a standard rule.?

@Eirik0 Eirik0 referenced this pull request Sep 27, 2018

Open

Look for TODOs and FIXME in code. #3546

20 of 25 tasks complete
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 27, 2018

The above sigops bug should be handled in a separate PR, as it is not directly caused by this PR (just maintained with Sapling), and can be circumvented by users in the short term by specifying a concrete maximum instead of zero.

I pushed a commit to address @Eirik0's comment.

@ebfull

ebfull approved these changes Sep 27, 2018

Copy link
Contributor

ebfull left a comment

ACK

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 28, 2018

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

str4d added some commits Sep 13, 2018

@str4d str4d force-pushed the str4d:3216-z_shieldcoinbase branch from 75c571a to 5f91a95 Oct 3, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 3, 2018

Rebased on master to fix merge conflict. I also squashed the minor fixup commit into the main one.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

Filed #3564 regarding the sigops limit.


def setup_chain(self):
print("Initializing test directory "+self.options.tmpdir)
initialize_chain_clean(self.options.tmpdir, 4)

def setup_network(self, split=False):
args = ['-regtestprotectcoinbase', '-debug=zrpcunsafe']
args2 = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', "-mempooltxinputlimit=7"]
if self.addr_type != 'sprout':

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

If there are future address types, Overwinter and Sapling should also be activated for them.

I suspect that Overwinter should be activated when the address type is "sprout", but that is covered by #2966 I think. (If I'm guessing correctly as to why "-mempooltxinputlimit=7" is no longer needed, that would also apply when Overwinter is activated but Sapling is not.)


def setup_chain(self):
print("Initializing test directory "+self.options.tmpdir)
initialize_chain_clean(self.options.tmpdir, 4)

def setup_network(self, split=False):
args = ['-regtestprotectcoinbase', '-debug=zrpcunsafe']
args2 = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', "-mempooltxinputlimit=7"]
if self.addr_type != 'sprout':

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

Looking at the remainder of the test, it definitely seems as though coverage would benefit from running it for all three cases (launch, post-Overwinter, post-Sapling).

@@ -1520,21 +1520,21 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
std::string mainnetzaddr = "zcMuhvq8sEkHALuSU2i4NbNQxshSAYrpCExec45ZjtivYPbuiFPwk6WHy4SvsbeZ4siy1WheuRGjtaJmoD1J8bFqNXhsG6U";

try {
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(mtx, {}, testnetzaddr, -1 ));
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, {}, testnetzaddr, -1 ));

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

This seems to be improving coverage for Sapling shielded transactions at the expense of coverage for Sprout shielded transactions :-( Ideally we'd test both.

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

No, this isn't the same mechanism as with z_sendmany (where the presence of TransactionBuilder indicates Sapling usage). Here, we only have a shielded output address, so we can simply run the Sprout-or-Sapling logic based on the output address type. The changes here are just to satisfy the new constructor argument requirements.

} catch (const UniValue& objError) {
BOOST_CHECK( find_error(objError, "Fee is out of range"));
}

try {
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(mtx, {}, testnetzaddr, 1));
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, {}, testnetzaddr, 1));
} catch (const UniValue& objError) {
BOOST_CHECK( find_error(objError, "Empty inputs"));
}

// Testnet payment addresses begin with 'zt'. This test detects an incorrect prefix.

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

While the first sentence is technically correct for Sapling testnet shielded addresses (which begin with "ztestsapling"), I'm skeptical that this actually tests what it claims, since the code here doesn't mention "zt".

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

This is still a Sprout test; see above comment.

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

I'm still skeptical that it tests what it claims, but OK, that means it's not a new issue for this PR. Maybe file a follow-up ticket.

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

The test uses SelectParams(CBaseChainParams::TESTNET) so it requires testnet addresses; the test below uses a mainnet Sprout address. This is effectively checking that DecodePaymentAddress() is being called correctly and having its errors returned.

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

Right, but the comment implies that the test would detect if a prefix not starting with "zt" were used, which can't be the case because the test doesn't mention "zt".

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

The test would detect this, because the address gets passed through to DecodePaymentAddress(), which enforces the zt prefix. The fact we're only passing in an address with a zc prefix doesn't change the fact that any other prefix would also fail.

@@ -86,7 +86,7 @@ class TransactionBuilder
uint256 ovk,
libzcash::SaplingPaymentAddress to,
CAmount value,
std::array<unsigned char, ZC_MEMO_SIZE> memo);
std::array<unsigned char, ZC_MEMO_SIZE> memo = {{0xF6}});

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

I would have expected this to cause the wallet_listreceived.py tests to fail, since this PR doesn't update them: https://github.com/zcash/zcash/blob/master/qa/rpc-tests/wallet_listreceived.py#L14-L16
Why is that not the case, and does it indicate a test coverage problem?

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

Oh, this is probably an actual issue. We ran try before the rebase, when the above RPC test was not in master (and thus not in this branch). I'll test locally, and push a change.

This comment has been minimized.

@str4d

str4d Oct 4, 2018

Contributor

Yep, breaks as expected! Pushed a fix.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 4, 2018

Re: the sigops bug, I think this is a sharp edge that should be addressed by reducing the default for now (pending a more complete solution in #3564). Otherwise I can see this being a common user-support problem.

@daira

daira approved these changes Oct 4, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov) f09aae0

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

(re-reviewing right now)

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 5, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

📌 Commit f09aae0 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

⌛️ Testing commit f09aae0 with merge c418594...

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

Auto merge of #3518 - str4d:3216-z_shieldcoinbase, r=bitcartel
Add Sapling support to z_shieldcoinbase

Part of #3216.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 5, 2018

Pushed a trivial commit to fix the pyflakes warnings.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

📌 Commit 089ec92 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

⌛️ Testing commit 089ec92 with merge 625797a...

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

Auto merge of #3518 - str4d:3216-z_shieldcoinbase, r=str4d
Add Sapling support to z_shieldcoinbase

Part of #3216.

@str4d str4d moved this from In Review to Merge Queue in Zcashd Team Oct 5, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 625797a to master...

@zkbot zkbot merged commit 089ec92 into zcash:master Oct 5, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from Merge Queue to Released (Merged in Master) Oct 5, 2018

@str4d str4d deleted the str4d:3216-z_shieldcoinbase branch Oct 8, 2018

@str4d str4d referenced this pull request Oct 8, 2018

Closed

Add Sapling support to wallet RPCs #3063

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