Orchard things that might have been missed

# Orchard things that might have been missed We are looking at commit 4339bd5ef227f67727b61e0454d45a1718e1a33a (feature/wallet_orchard-tx_builder on str4d's fork). ``` src$ grep --context=50 -Irni --exclude='*.Po' sapling |tee ../sapling-50-context.cpp ``` **WARNING:** We missed some things because my search for sprout wasn't always case insensitive. We went through everything again, up to ==cursor==. Search case-insensitively for "Sprout" * `librustzcash_init_zksnark_params`: no params for Orchard * other librustzcash.h: probably fine * `transaction_ffi.rs`:48 - code explicitly for handling pre-NU5 placeholder wtxids * `transaction_ffi.rs`:83 - documented to return nullptr for v1-v4 transactions, and code does that * `rustzcash.rs`: fine because Orchard stuff is in `orchard_ffi.rs` * `history_ffi.rs`:10 - the default case handles NU5 and later * `txdb.h`84 - has Orchard * `transaction_builder.h`:311 - ok, Orchard spends were implemented later * skipping filenames in Makefiles * `primitives/transaction.cpp`:130,187 - default argument for constructor is fine * `primitives/transaction.h`:81 - `requireSprout` is just to work out whether we need to use a v4 transaction * `primitives/transactions.h`:726 - implemented for NU5 * `primitives/tx_version_info.cpp`22 -- implemented for NU5 * `./chain.h`:18 -- implemented for NU5 * `./txdb.cpp`:23 - type tags for Orchard are present * `./txdb.cpp`:60,265 - implemented for Orchard * `./test/coins_test.cpp`:30-284 -- all implemented for Orchard * `./test/test_bitcoin.cpp`:44 -- just parameters * `./chainparams.cpp`:110,393,645,739 -- just chainparams stuff, NU5 activation height set in the future * `./script/interpreter.h,cpp` -- this is sprout as in the name of the pre-overwinter sighash algorithm * `./proof_verifier.cpp` -- this is the sprout proof verifier, presumably implemented in Rust for Orchard elsewhere * `./txmempool.cpp`:132,304 -- implemented for Orchard * `txmempool.cpp`:420,656,855,936 -- implemented for orchard * `main.cpp`: * `ShieldRejectCode` ok * `ShieldedReqRejectReason` ok * L1347 CheckTransaction ok * L1818 was missing an orchard check, but [PR #5788](https://github.com/zcash/zcash/pull/5788/files#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2L1836-L1847) removes the nullifier checks in AcceptToMemoryPool * L3045 negative shielded value pool balances ok * L3118 ok * L3265 ok * L3302 ok * construct incremental Merkle tree ok * add outputs ok * L4368 `ReceivedBlockTransactions` ok * L4328 Sprout-specific ok * L5189 and all of LoadBlockIndexDB ok * `rpc/blockchain.cpp`:268, 1079 value pools ok * `miner.h`:37 ok * `transaction_builder.cpp`:159 we have this for orchard just not here * `transaction_builder.cpp` we know we can build orchard transactions * `rpc/blockchain.cpp`:1225 `z_gettreestate` wass not implemented for Orchard but now is (PRs #5560 and #5852) * `rpc/rawtransaction.cpp`:64 TxJoinSplitToJSON is sprout-specific, and getrawtransaction isn't implemented for Sapling and so is deprecated * `consensus/upgrades.cpp`:14 - has NU5 * `consensus/upgrades.cpp`:59, 67 - everything is good here (just a constant) * `consensus/params.h`:30 - enum is good * `consensus/consensus.h` - just constants, ZIP225 constants correct * `validationinterface.h`:40,62 - this was missing but it's fixed in the latest master branch * `miner.cpp`: * L458 value pools ok * L542 turnstile violation ok * L713 this was missing Orchard support but it has been added * `utiltest.cpp`:15, rest of file - this is a helper function for tests * `coins.cpp`:47, 48-216, 217, 307-667, 775, 883, 935, 947, 997 -- implemented for Orchard * `coins.cpp`:668 -- wasn't implemented for Orchard, but is now * `coins.h`: 553,636 -- just a signature * ==cursor okay== * `gtest/test_merkletree.cpp` appears to be implemented for Orchard, but double check all tests are there * `gtest/main.cpp`:18 parameters ok * `init.cpp`: 707,1133 parameters ok * `keystore.h` appears to be implemented for UAs * `validationinterface.cpp` implemented for Orchard * `policy/policy.cpp`:3013 implemented for NU5 * `txmempool.h` has orchard * `utiltest.h` looks like just helper functions for tests * `keystore.cpp` looks implemented for UAs * ==cursor okay 2== * ==where Daira and Taylor's pairing left off== * `zcash/Address.hpp:14` just #includes Things to look into: * `transaction_ffi.rs`:141 -- missing documentation that `zcash_transaction_transparent_signature_digest` doesn't support v1-v4 * `builder_ffi.rs`:144 -- missing documentation that `zcash_builder_zip244_shielded_signature_digest` doesn't support v1-v4 * `transaction_builder.h`: no `orchardChangeAddr` (probably because you can't override the change addr for Orchard) * `primitives/transaction.h`:331 and below -- check that this is implemented for Orchard in Rust * `./chain.h`:244,330,518 and `txdb.cpp`:594 -- missing saplingAnchor and orchardAnchor -- guess: either implemented somewhere else, or not needed because related to Sprout interstitial treestates * `test/sighash_tests.cpp`:171 -- this test code is generating a random tx that never includes Orchard fields * `test/coins_tests.cpp`:285 `// TODO: Orchard nullifier` * `test/coins_tests.cpp`:293 and below -- test utilities that don't include Orchard (maybe related to other items for this file) * `test/coins_tests.cpp`:311 `checkNullifierCache()` * `test/coins_tests.cpp`:483 - missing Orchard test in `anchor_pop_regression_test` (maybe this would have found the `PopAnchor` bug!) * `test/coins_tests.cpp`:575,63,796 - missing Orchard tests in `anchor_regression_test`, `anchors_flush_test`, `anchors_test` * `test/key_tests.cpp`:193-end - Make sure we have these tests for unified addresses. Sapling test is missing for viewing keys. * `test/transaction_tests.cpp` - Make sure we have equivalent testing for v5 transactions. * `txmempool.cpp`:374 - does not handle ORCHARD case * `txmempool.cpp`:580 - there are asserts for sprout and sapling but they are missing for orchard * `txmempool.cpp`645 - checkNullifiers is called for SPROUT and SAPLING but not ORCHARD. Fixed by PR [#5788](https://github.com/zcash/zcash/issues/5788) * `main.cpp`: * ConextualCheckTransaction - consensus rule review in https://hackmd.io/ZqH-UTkNQ4usFvOxGBL0Ig TODO: Also follow this consensus rule review process for CheckTransactionWithoutProofVerification * L2897 missing `PopAnchor`!!! :-) * L3644, L3751 `PoolMetrics` (definitely wrong [#5880](https://github.com/zcash/zcash/issues/5880)) * L3767 `DisconnectTip` * `transaction_builder.cpp`:376 `SendChangeTo[Sprout]` probably ok * `rpc/misc.cpp`:268 unified addresses TODO * `rpc/misc.cpp`:276 unified addresses (no doc) * ==cursor to look into== * `gtest/test_dynamicusage.cpp`: no tests for Orchard * `gtest/test_checktransaction.cpp` no tests for Orchard, **we should at least implement tests for invalid Orchard shielded coinbase transactions** * `gtest/test_feature_flagging.cpp`: missing tests for Orchard/NU5 * `gtest/test_history.cpp`: missing tests for Orchard/NU5 and maybe other NUs, on line 21 there is a switch that is missing the UPGRADE_NU5 case * `gtest/test_validation.cpp`: missing tests for Orchard/NU5 * `gtest/test_mempool.cpp`: might be missing NU5 tests * `gtest/test_keystore.cpp` might be missing some tests, e.g. keystore with Orchard * ==where Daira and Taylor's pairing left off== * `gtest/test_keystore.cpp`: no tests for Orchard * `gtest/test_transaction_builder.cpp:66` mock function doesn't take orchard arguments -- is the thing it's mocking also missing those? * `gtest/test_transaction_builder.cpp`: only has TransparentToOrchard tests, missing TransparentToSapling, SproutToSproutAndOrchard, etc.? * `gtest/test_checkblock.cpp` missing NU5 tests * `zcbenchmarks.h:15`: missing benchmark_try_decrypt_orchard_notes and benchmark_increment_orchard_note_witnesses * `History.cpp` I'm not sure * `Note.cpp` I'm not sure * ==cursor to look into 2== * `zcash/Address.hpp:105` missing address-related functions for orchard which is intentional (double-check because I'm not sure) * `zcbenchmarks.cpp`: this file is where the benchmark_* functions mentioned above would be implemented Things to look into that I (Taylor) found by skimming. This is in order but has unmentioned omissions (also oops my search for sprout wasn't always case insensitve). I separated it out to make sure the lists above have no omissions. * `wallet/asyncoperation_mergetoaddress.cpp:60` AsyncRPCOperation_mergetoaddress doesn't take an orchardNoteInputs argument * `wallet/asyncoperation_mergetoaddress.cpp:226,409,etc.` implementation of mergetoaddress doesn't do orchard * `wallet/rpcdump.cpp:659` orchard z keys not dumped (is that because they're always derived from the seed?) * `wallet/rpcdump.cpp:708` RPC docs don't mention orchard, should explain if/how the orchard secrets are included in the dump * Did/should we change automigration to be Sprout->Orchard instead of Sprout->Sapling? * `wallet/test/rpc_wallet_tests.cpp:408` missing z_validateaddress tests with UAs * `wallet/test/rpc_wallet_tests.cpp:470` missing z_importkey for UA? -- probably intentional * ==skipped over a bunch of tests== * `wallet/rpcwallet.cpp:328` listaddress output doesn't include UAs * `wallet/rpcwallet.cpp:2368` orchardEntries is unused, unspent orchard notes therefore aren't included in z_listunspent's output * `wallet/rpcwallet.cpp:3398` orchard balance isn't added to the sum * `wallet/rpcwallet.cpp:3589` `//TODO orchard` * ==skipping rpcwallet.cpp since i assume this is just not-yet-implemented stuff (lots of RPCs are missing orchard)== * `wallet/wallet.h:795` SpendableInputs doesn't include orchard notes' balance in the sum * `wallet/wallet.h:1013` IncrementNoteWitnesses doesn't take an orchard argument. **It's a privacy leak if we don't increment orchard witnesses** (ah, it doesn't take an argument because it uses a global variable; we should still make sure witness incrementation is implemented though. it calls orchard_wallet_append_bundle_commitments FFI, I can't find the rust code). I recommend adding a test that merkle paths are always recent. * `wallet/wallet.cpp:6109` `/* TODO ORCHARD */` in GetFilteredNotes() * `wallet/wallet.cpp:6677` LimitToAmount selects transparent, sprout, sapling outputs but not orchard * This is used by sendmany which (as opposed to z_sendmany) will mix transparent and shielded outputs. **The docs don't warn about this, so it's potentially a privacy leak** (if the user thinks it's only going to send transparent funds). * `wallet/wallet.cpp:6749` LogInputs() logs transparent, sprout, sapling, but not orchard Other: * `test/test_bitcoin.cpp`:44 `JoinSplitTestingSetup` should be called `ShieldedTestingSetup` * `wallet/rpcwallet.cpp:4736` "finalized" transaction depth of 10 is hard-coded, should be a constant otherwise we'll forget to change it there Other stuff we should do: - Search for TODOs - look for integer overflow in miner.cpp:545 - **we stopped at line 21021 of the context file (main() of gtest)** - We should search for code where NU names appear nearby, e.g. "Heartwood", "Canopy" and ensure that the same or similar logic is implemented for "NU5" Limitations: - This search might miss things that need to be done for Sapling and Orchard but not Sprout, e.g. because they are related to features that Sprout doesn't have (such as full viewing keys or diversified addresses).