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

z_sendmany error: Witness for spendable note does not have same anchor as change input #1823

Closed
etherchain-org opened this issue Nov 8, 2016 · 14 comments
Labels
C-bug Category: This is a bug I-error-handling Problems and improvements related to error handling I-protocol-fragility Problems and improvements with respect to protocol fragility. M-has-pr To-be-removed (GitHub has linked:pr filter) note selection and shielded tx construction
Milestone

Comments

@etherchain-org
Copy link

I receive the following error when trying to send from a z address to 10 other z addresses.

I am using the standard z_sendmany API. How are we supposed to transact with z addresses in an efficient way?!

@bitcartel
Copy link
Contributor

@etherchain-org Which version of zcashd are you using? v1.0.1 or v1.0.2? Do you use debug=zrpc or debug=all when launching zcashd? Can you share some of the debug output related to the z_sendmany operation?

@etherchain-org
Copy link
Author

I am using v1.0.2. Unfortunately I am not having any debug logging flags enabled and I would prefer not to restart the node as that will take it offline for several hours. Is there any way to change the logging level on the fly?

@daira daira added C-bug Category: This is a bug I-error-handling Problems and improvements related to error handling note selection and shielded tx construction I-protocol-fragility Problems and improvements with respect to protocol fragility. needs prioritization labels Nov 9, 2016
@nathan-at-least nathan-at-least added this to the 1.0.4 milestone Nov 14, 2016
@bitcartel
Copy link
Contributor

@etherchain-org I don't believe there is a way to change the logging level on the fly. We have not added one. Have you updated to v1.0.3 on one of your nodes and does this problem persist?

@etherchain-org
Copy link
Author

@bitcartel I have updated my node to v1.0.3 and the issue persists.

@bitcartel
Copy link
Contributor

Another user reports:

z to z transactions are still failing: async rpc opid-f3f8a153-5e7e-4b2d-b0e2-5791d9aee7bb finished (status=failed, error=Witness for spendable note does not have same anchor as change input)
was sending from one z address to 10 other z addresses

@bitcartel
Copy link
Contributor

@etherchain-org As a data point, when you updated your node to v1.0.3, did you do a -reindex?

@peterbitfly
Copy link

@bitcartel I did not do a -reindex as it takes quite a lot of time with the z addresses in my wallet

@str4d
Copy link
Contributor

str4d commented Dec 1, 2016

I think that the bug is caused by a difference in anchors between the change and non-change input notes.

Let's assume that we want to send 1.5 ZEC each to four people, and we only have 1 ZEC notes. The resulting multi-JoinSplit will look like this:

  • JS1
    • In: 1, 1
    • Out: 1.5, 0.5 (a)
  • JS2
    • In: 0.5 (a), 1
    • Out: 1.5, 0 (b)
  • JS3
    • In: 1, 1
    • Out: 1.5, 0.5 (c)
  • JS4
    • In: 0.5 (c), 1
    • Out: 1.5, 0 (d)

For JS1, the anchor is the one corresponding to the fetched witnesses for the input notes. For JS2, it is the anchor corresponding to the tree state after appending the JS1 commitments to the JS1 anchor's tree state (a block tree state). Because the JS2 change is 0, the JS3 anchor is taken from a block state again.

Here's where I think the bug comes in:

  • For JS4's change input note, its witness is calculated by taking the tree state for JS3's anchor, and appending the JS3 commitments.
  • For JS4's non-change input notes, their witnesses are calculated by fetching witnesses for a block tree state, and appending the JS3 commitments.

But we don't lock on cs_main between these steps, or between JoinSplits (obviously because proof generation takes a while). So, what I think is happening is that JS3 is created with a particular block anchor, and then another block arrives while its proof is being calculated, and then the non-change input notes for JS4 use the anchor from the next block. This causes different commitment trees to be created for the change and non-change input notes, resulting in the given error. The bug primarily affects many-z-addr transactions, because the the transaction creation time approaches or exceeds the block interval time.

@str4d
Copy link
Contributor

str4d commented Dec 1, 2016

Thinking more about this, you could remove JS1 and JS2, and the same argument would apply. I included JS1 and JS2 because originally I thought this bug was caused by the intermediate tree state being cleared, which is not well-specified in the protocol spec.

@bitcartel
Copy link
Contributor

bitcartel commented Dec 1, 2016

I'll try to make a test for this. For each joinsplit in a chain, if there non-change notes to spend, we lock on cs_main to call pwalletMain->GetNoteWitnesses. So it may be that instead we need to retrieve all input note witnesses before we start processing the chain.

@str4d
Copy link
Contributor

str4d commented Dec 2, 2016

For each joinsplit in a chain, if there non-change notes to spend, we lock on cs_main to call pwalletMain->GetNoteWitnesses. So it may be that instead we need to retrieve all input note witnesses before we start processing the chain.

That would make the most sense to me, and would probably be the simplest to implement (via a zInputWitnessesDeque parallel to zInputsDequeue).

@bitcartel
Copy link
Contributor

Quick update: I managed to recreate this problem manually. Developing test and fix right now.

bitcartel added a commit to bitcartel/zcash that referenced this issue Dec 3, 2016
@bitcartel bitcartel added the M-has-pr To-be-removed (GitHub has linked:pr filter) label Dec 3, 2016
@daira
Copy link
Contributor

daira commented Dec 8, 2016

I remain convinced that choosing the most recent anchor for a JoinSplit is not a good idea.

@bitcartel
Copy link
Contributor

That would have to be a separate PR. Currently the anchor is obtained by calling GetNoteWitnesses() on the wallet, but there is no way to specify what type of anchor to retrieve e.g. oldest, at height x, or n confirmations ago.

zkbot pushed a commit that referenced this issue Dec 8, 2016
…r_as_change_input, r=str4d

With chained joinsplits, witness anchors for input notes no longer cross block boundaries

Closes #1823
jmprcx added a commit to z-classic/zclassic that referenced this issue Dec 19, 2016
* Add getlocalsolps and getnetworksolps RPC calls, show them in getmininginfo

* Add benchmark for attempting decryption of notes

* Add benchmark for incrementing note witnesses

* Add -metricsui flag to toggle between persistent screen and rolling metrics

Defaults to true if stdout is a TTY, else false.

* Add -metricsrefreshtime option

* Only show metrics by default if stdout is a TTY

* Document metrics screen options

* Fix stale comment referencing upstream block interval

* Add checkpoint at block height 15000

* Added mainnet, testnet, and onion nodes

* Make command line option to show all debugging consistent with similar options

Most people expect a value of 1 to enable all for command line arguments.
However to do this for the -debug option you must type "-debug=".
This has been changed to allow "-debug=1" as well as "-debug=" to
enable all debug logging

* Update documentation to match the zcash#4219 change

* Update help message to match the zcash#4219 change

* Clarify that metrics options are only useful without -daemon and -printtoconsole

* Increase length of metrics divider

* Closes zcash#1857. Fixes bug where tx spending only notes had priority of 0.

* Closes zcash#1901. Increase default settings for the max block size when
mining and the amount of space available for priority transactions.

* Write witness caches when writing the best block

For steady-state operation, this reduces the average time between wallet disk
writes from once per block to once per hour.

On -rescan, witness caches are only written out at the end along with the best
block, increasing speed while ensuring that on-disk state is kept consistent.

Witness caches are now never recreated during a -reindex, on the assumption that
the blocks themselves are not changing (the chain is just being reconstructed),
and so the witnesses will remain valid.

Part of zcash#1749.

* Add porter dev overrides for CC, CXX, MAKE, BUILD, HOST

* Apply miniupnpc patches to enable compilation on Solaris 11

These can be removed after the next MiniUPnP release.

Closes zcash#1835.

* Closes zcash#1903. Add fee parameter to z_sendmany.

* Add an upstream miniupnpc patch revision

* Metrics - Don't exclaim unless > 1

"You have validated 0 transactions!" sounds a little less enthusiastic that intended. Also, only says "1 transaction".

* Address review comments, tweak strings

* bash-completion: Adapt for 0.12 and 0.13

 * separate completion for bitcoind and bitcoin-cli
 * remove RPC support from bitcoind completion
 * add completion for bitcoin-tx and bitcoin-qt
 * rely on autoloading of completions

* Change function names to not clash with Bitcoin, apply to correct binaries

* Add bash completion files to Debian package

* Always bash-complete the default account

* Add Zcash RPC commands to CLI argument completion

* Fixes zcash#1823. Witness anchors for input notes no longer cross block boundaries.

* Edit for grammar: "block chain"

At this point, I believe it is universally accepted that "blockchain" is one word, and should not be separated into two.

* Increase timeout as laptops on battery power have cpu throttling.

* Isolate verification to a `ProofVerifier` context object that allows verification behavior to be tuned by the caller.

* Regression test.

* Ensure cache contains valid entry when anchor is popped.

* Ensure ProofVerifier cannot be accidentally copied.

* Document behaviour of CWallet::SetBestChain

* WitnessAnchorData only needs to store one witness per JSOutPoint.

* Rename Dummy to Disabled.

* Add more tests for ProofVerifier.

* Fix indentation

* Generate JS for trydecryptnotes, make number of addresses a variable

* Add JS to second block to ensure witnesses are incremented

* ASSERT_TRUE -> ASSERT_FALSE

* Skip JoinSplit verification before the last checkpoint

Part of zcash#1749

* Gather release notes from previous release to HEAD

Also update release-process.md to replace git shortlog command with
release-notes.py script.

* Add a reindex test that fails because of a bug in decrementing witness caches

Ref: zcash#1904 (comment)

* Make the test pass by fixing the bug!

* Only check cache validity for witnesses being incremented or decremented

Fixes the bug resulting from zcash#1904.

* Check that E' points are actually in G2 by ensuring they are of order r.

* Fix bug in wallet tests

* Extract block-generation wallet test code into a function

* Rewrite reindex test to check beyond the max witness cache size

* Fix bug in IncrementNoteWitness()

* Update payment API docs to recommend -rescan for fixing witness errors

* Update version to 1.0.4

* Update man pages

* Release notes, authors, changelog

* Update seed nodes

* Bugfix #14 - getblocksubsidy RPC command is incorrect
jmprcx added a commit to z-classic/zclassic that referenced this issue Jan 31, 2017
* Add getlocalsolps and getnetworksolps RPC calls, show them in getmininginfo

* Add benchmark for attempting decryption of notes

* Add benchmark for incrementing note witnesses

* Add -metricsui flag to toggle between persistent screen and rolling metrics

Defaults to true if stdout is a TTY, else false.

* Add -metricsrefreshtime option

* Only show metrics by default if stdout is a TTY

* Document metrics screen options

* Fix stale comment referencing upstream block interval

* Add checkpoint at block height 15000

* Make command line option to show all debugging consistent with similar options

Most people expect a value of 1 to enable all for command line arguments.
However to do this for the -debug option you must type "-debug=".
This has been changed to allow "-debug=1" as well as "-debug=" to
enable all debug logging

* Update documentation to match the zcash#4219 change

* Update help message to match the zcash#4219 change

* Clarify that metrics options are only useful without -daemon and -printtoconsole

* Increase length of metrics divider

* Closes zcash#1857. Fixes bug where tx spending only notes had priority of 0.

* Closes zcash#1901. Increase default settings for the max block size when
mining and the amount of space available for priority transactions.

* Write witness caches when writing the best block

For steady-state operation, this reduces the average time between wallet disk
writes from once per block to once per hour.

On -rescan, witness caches are only written out at the end along with the best
block, increasing speed while ensuring that on-disk state is kept consistent.

Witness caches are now never recreated during a -reindex, on the assumption that
the blocks themselves are not changing (the chain is just being reconstructed),
and so the witnesses will remain valid.

Part of zcash#1749.

* Add porter dev overrides for CC, CXX, MAKE, BUILD, HOST

* Apply miniupnpc patches to enable compilation on Solaris 11

These can be removed after the next MiniUPnP release.

Closes zcash#1835.

* Closes zcash#1903. Add fee parameter to z_sendmany.

* Add an upstream miniupnpc patch revision

* Metrics - Don't exclaim unless > 1

"You have validated 0 transactions!" sounds a little less enthusiastic that intended. Also, only says "1 transaction".

* Address review comments, tweak strings

* bash-completion: Adapt for 0.12 and 0.13

 * separate completion for bitcoind and bitcoin-cli
 * remove RPC support from bitcoind completion
 * add completion for bitcoin-tx and bitcoin-qt
 * rely on autoloading of completions

* Change function names to not clash with Bitcoin, apply to correct binaries

* Add bash completion files to Debian package

* Always bash-complete the default account

* Add Zcash RPC commands to CLI argument completion

* Fixes zcash#1823. Witness anchors for input notes no longer cross block boundaries.

* Edit for grammar: "block chain"

At this point, I believe it is universally accepted that "blockchain" is one word, and should not be separated into two.

* Increase timeout as laptops on battery power have cpu throttling.

* Isolate verification to a `ProofVerifier` context object that allows verification behavior to be tuned by the caller.

* Regression test.

* Ensure cache contains valid entry when anchor is popped.

* Ensure ProofVerifier cannot be accidentally copied.

* Document behaviour of CWallet::SetBestChain

* WitnessAnchorData only needs to store one witness per JSOutPoint.

* Rename Dummy to Disabled.

* Add more tests for ProofVerifier.

* Fix indentation

* Generate JS for trydecryptnotes, make number of addresses a variable

* Add JS to second block to ensure witnesses are incremented

* ASSERT_TRUE -> ASSERT_FALSE

* Skip JoinSplit verification before the last checkpoint

Part of zcash#1749

* Gather release notes from previous release to HEAD

Also update release-process.md to replace git shortlog command with
release-notes.py script.

* Add a reindex test that fails because of a bug in decrementing witness caches

Ref: zcash#1904 (comment)

* Make the test pass by fixing the bug!

* Only check cache validity for witnesses being incremented or decremented

Fixes the bug resulting from zcash#1904.

* Update release process to check in with users who opened resolved issues

* Check that E' points are actually in G2 by ensuring they are of order r.

* Fix bug in wallet tests

* Extract block-generation wallet test code into a function

* Rewrite reindex test to check beyond the max witness cache size

* Fix bug in IncrementNoteWitness()

* Extend createjoinsplit to benchmark parallel JoinSplits

Closes zcash#1940

* Update payment API docs to recommend -rescan for fixing witness errors

* Add total number of commitments to getblockchaininfo

* Update version to 1.0.4

* Update man pages

* Release notes, authors, changelog

* Only enable getblocktemplate when wallet is enabled

* Only run wallet tests when wallet is enabled

* Add a tool for profiling the creation of JoinSplits

* Add test for IncrementalMerkleTree::size().

* Exclude test binaries from make install

Closes zcash#1943.

* Fixes zcash#1964 to catch general exception in z_sendmany and catch
exceptions as reference-to-const.

* Fixes zcash#1967 by adding age of note to z_sendmany logging.

* Scan the whole chain whenever a z-key is imported

Closes zcash#1941.

* Instruct users to run zcash-fetch-params if network params aren't available

Closes zcash#1786.

* Fixes a bug where the unsigned transaction was logged by z_sendmany
after a successful sign and send, meaning that the logged hash fragment
would be different from the txid logged by "AddToWallet".  This issue
occured when sending from transparent addresses, as utxo inputs must be
signed.  It did not occur when sending from shielded addresses.

* Trigger metrics UI refresh on new messages

* Strip out the SECURE flag in metrics UI so message style is detected

* Add 'CreateJoinSplit' standalone utility to gitignore.

* Handle newlines in UI messages

* Suggest ./zcutil/fetch-params.sh as well

Once we improve the from-source installation docs to use 'make install', we can
revert this commit.

* Update debug categories

Closes zcash#1954.

* CreateJoinSplit: add start_profiling() call

This solves the problem of profiling output displaying nonsensical large time values.

* rpc: Implement random-cookie based authentication

When no `-rpcpassword` is specified, use a special 'cookie' file for
authentication. This file is generated with random content when the
daemon starts, and deleted when it exits. Read access to this file
controls who can access through RPC. By default this file is stored in
the data directory but it be overriden with `-rpccookiefile`.

This is similar to Tor CookieAuthentication: see
https://www.torproject.org/docs/tor-manual.html.en

Alternative to zcash#6258. Like that pull, this allows running bitcoind
without any manual configuration. However, daemons should ideally never write to
their configuration files, so I prefer this solution.

* Rename build-aux/m4/bitcoin_find_bdb48.m4 to remove version

Closes zcash#1622.

* Bump COPYRIGHT_YEAR from 2016 to 2017.

* Throw an error if zcash.conf is missing

An empty zcash.conf is sufficient to bypass this error.

* Show a friendly message explaining why zcashd needs a zcash.conf

* Closes zcash#1780. Result of z_getoperationstatus now sorted by creation time of operation

* Create ISSUE_TEMPLATE.md

* move template to subdirectory, fix typo, include prompt under describing issue section, include uploading file directly to github ticket as option for sharing logs

* Remove UTF-8 BOM efbbbf from zcash.conf to avoid problems with command line tools

* Closes zcash#1097 so zcash-cli now displays license info like zcashd.

LicenseInfo is refactored from init.cpp to util.cpp so that the
bitcoin-cli makefile target does not need to be modified.

* Fixes zcash#1497 ZCA-009 by restricting data exporting to user defined folder.

Previously the RPC interface allowed z_exportwallet, backupwallet and
dumpwallet to write data to an arbitrary filename.  ZCA-009 demonstrates
how this is vulnerable.  The resolution is to only allow data to
written when the -exportdir has been configured.  Also filenames are
restricted to alphanumeric characters.

* Closes zcash#1957 by adding tx serialization size to listtransactions output.

* Fix gtest ordering broken by zcash#1949

Part of zcash#1539

* Fixes zcash#1960: z_getoperationstatus/result now includes operation details.

* Debian package lint

- Tweak description synopsis to make Debian happy
- Put bash completion files in correct directory
- Add a manpage for zcash-fetch-params

* Generate Debian control file to fix shlibs lint

* Create empty zcash.conf during performance measurements

* Create empty zcash.conf during coverage checks

Fixes regression caused by zcash#2013.

* Coverage build system tweaks

* Update walletbackup.py qa test to use -exportdir option

* Add missing header required by std::accumulate

* Increase timeout for z_sendmany transaction in wallet.py qa test

* Add test for z_importkey rescanning from beginning of chain.

* Bump version to 1.0.5.

* Update release notes and Debian package.

* V1.0.4 mac (#51)

* initial mac version of zclassic

Work in progress - 15JAN2017

more refactoring

linux refactoring fixes

osx refactoring fixes

initial win64 commit

fixup! initial win64 commit

compile libsnark with posix threads

build gtest and gmock with posix

Working build

fixup! Working build

* Windows and Linux builds ok

* fixup! Merge tag 'v1.0.5' into v1.0.5-multios

* fixup! fixup! Merge tag 'v1.0.5' into v1.0.5-multios

* fixup! fixup! fixup! Merge tag 'v1.0.5' into v1.0.5-multios

* Fix OSX compatibility with depends

* OSX Compat - Fix site_t ambiguity in json

* fixup! OSX Compat - Fix site_t ambiguity in json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug I-error-handling Problems and improvements related to error handling I-protocol-fragility Problems and improvements with respect to protocol fragility. M-has-pr To-be-removed (GitHub has linked:pr filter) note selection and shielded tx construction
Projects
None yet
Development

No branches or pull requests

6 participants