Sapling Integration: Chain + Wallet transaction manager. #1798

furszy · 2020-08-10T15:57:57Z

Sapling integration, milestone 3 and 4.

Essentially, Sapling running on the regtest network -- yeah :), 🎉 🎉 -- and being covered by a good amount of unit and functional tests . (Still, not enough tests. We definitely need more).
This is the last big PR for Sapling work that have (fulfilling the complete base protocol functionality). All of the remaining work can be covered in smaller PRs.

Covering the following points:

Sapling transaction data:

Signature hash personalization + sigversion (custom ZIP243). 
Sapling transaction primitive data (OutputsDescription, SpendDescription, value and bindingSig)
Transaction version bump, Sapling will be using version 2.

Sapling transactions management:

Wallet: notes, nullifiers and witnesses data management.
Shielded transaction builder. 
Wallet Shielded transaction creation flow.
Wallet's transactions in-chain detection (note decryption + addToWalletIfInvolveMe).
Wallet's transactions rescan.
SaplingOperation class to decouple the Shielded tx creation + broadcast flow from the wallet sources.

Blockchain:

Incremental merkle integration for Sapling notes (sapling tree root hash stored on the block header). 
Chain: final merkle tree root calculation. 
Nullifiers & Anchors db, coins view integration.
Block version bump + miner v8 block generation. 
spent/unspent notes validation.
sapling transaction validation (sapling_validation.h). 
Negative shielded pool value detection (zip209). 
Coins: check transaction shielded requirements validation.

RPC Server:

New Commands:

getshieldedbalance
listshieldedunspent
shielded_sendmany
exportsaplingkey
importsaplingkey
importsaplingkey
importsaplingviewingkey
exportsaplingviewingkey
listreceivedbyshieldedaddress
viewshieldedtransaction
getsaplingnotescount

Modified Commands:

dumpwallet
importwallet

Unit test coverage:

Sapling test fixture. 
Coins nullifiers & anchors check in coins_tests. 
sighash_tests checking the Sapling signature.
transaction builder tests. 
rpc wallet sapling tests.
wallet import/export keys test.
wallet keys encryption/decryption test.
sapling_wallet_tests (coverage for internal wallet methods: FindMySaplingNotes, SetSaplingNoteAddrsInCWalletTx, GetConflictedSaplingNotes, SaplingNullifierIsSpent, NavigateFromSaplingNullifierToNote, SpentSaplingNoteIsFromMe, CachedWitnessesEmptyChain, CachedWitnessesChainTip, CachedWitnessesDecrementFirst, CachedWitnessesCleanIndex, ClearNoteWitnessCache, UpdatedSaplingNoteData, MarkAffectedSaplingTransactionsDirty).

Functional tests:

sapling_wallet
sapling_wallet_anchorfork 
sapling_wallet_nullifiers 
sapling_wallet_listreceived 
sapling_key_import_export
sapling_changeaddresses

TODO, upcoming work:

Connect txmempool nullifiers. 
Fix memo field issues. 
Cache shielded credit/debit amounts.
Main.cpp validations further review.
Wallet: implement locked notes. 
Wallet: back port clear witness caches.
Rewind to last NU functionality.
Cleanup sprout code (remnant, not used).
Connect GUI.

Need further discussion:

Shielded transactions fees (current placeholder hardcoded to 1 PIV).
Mixed transactions validation (Transaction with transparent and shielded inputs/outputs). Currently these type of transaction are being checked on the sapling_validation file, as well as the version 2 tx with purely transparent inputs and outputs.
We need to think whether should decouple the net validations commits from this PR or not (those who touches the network consensus). It's a hard choice as the vast majority of unit and functional tests validating the correct behavior comes by the middle-end of this work. --- Hopefully commits are atomically enough to make the review process easier ---

...........................................................................

Work based on the ECC implementation. It's NOT one-to-one. Have adapted it into our network + modified the architecture and improved specific areas of the sources. There is still lot of work to do, plenty of space for improvements :) .

...........................................................................

Sorry for the massive PR but couldn't resist myself 🙏 . We can definitely think on how to decouple it in smaller PRs if things get hard to review. As said above, commits should be atomic enough but i know that it's not, commit wise, a 100% incremental work.

2fff9ec GetShieldedSpendsHash & GetShieldedOutputsHash assert txTo.sapData existence. (furszy) 3219140 transaction primitive: cleaning a weird GetTransaction function in the .cpp file. (furszy) ac88244 Validation: guard v2 shielded transaction serialization for pre-v5 upgrade window. (furszy) c613b4f Rename SIGVERSION_WITNESS_V0 to SIGVERSION_SAPLING. (furszy) 831f742 Not try to serialize empty fields in Sapling signature hash (furszy) d73ed0a Sapling signature hash (Custom version of ZIP 243) (furszy) b3cfc3c Introduce SignatureHash personalization updates. (furszy) 5e9a0d3 transaction: fix sapling version to 2. (furszy) 6d04323 make SaplingTxData Optional in transaction primitive (furszy) 34ef66a Transaction: add sapling data to toString (furszy) 3ed6bca Missing SaplingOutPoint::ToString() method implemented. (furszy) e8c0b48 [Sapling] Transaction GetShieldedValueIn & GetValueOut back port (furszy) 4ab08fa Fix sighash_tests. (furszy) cf77a5d sapling_transaction.h migrated to the new serialization (furszy) 12fc142 Add serialization for std:list objects. (furszy) abba85d Basic Sapling transaction primitive data. (furszy) fc3444c Backport zkSNARK compression. (furszy) b000a6f Transaction: Implement BaseOutPoint class. (furszy) 1d07756 Add serialization for primitive boost::optional<T>. (furszy) Pull request description: Work coming from #1798. Decoupled commits related to the Sapling transaction primitive data and the new signature hash to make the review a little bit easier there. Plus, squashed some of them. List of commits taken from #1798: Primitive data: * Add serialization for primitive boost::optional<T>. —> 659cc55 * Transaction: Implement BaseOutPoint class. —> f17b23c * Backport zkSNARK compression. —> 5dbc3c9 * Basic Sapling transaction primitive data --> c49594f * Add serialization for std:list objects. —> 7cc57f7 * sapling_transaction.h migrated to the new serialization —> dbb618f * Fix sighash_tests —> e1e3316 * Transaction GetShieldedValueIn & GetValueOut back port —> 0fbe023 * BugFix in GetShieldedValueIn —> 57ee1bc * Missing SaplingOutPoint::ToString() method implemented —> e1fe5bf7 * Transaction: add sapling data to toString —> 26ce38b * make SaplingTxData Optional in transaction primitive — PARTIALLY —> c3b1aed * Cleaning compiler warnings — PARTIALLY —> 644c381 * transaction: fix sapling version to 2. —> 8fc595f * Transaction: GetDustThreshold method implemented. —> dadf199. * Transaction:hasSaplingData check is enough checking for vShieldedOutput and vShieldedSpend emptiness. —> 57d3236 Signature hash: * Introduce SignatureHash personalization updates —> 550217f * Enable sigversion for Sapling transactions. —> c04abe1 * Sapling signature hash (Custom version of ZIP 243) —> eac206e * Not try to serialize empty fields in Sapling signature hash ---> ce5a467 ACKs for top commit: random-zebra: Nice. 🍻 ACK 2fff9ec Fuzzbawls: utACK 2fff9ec Tree-SHA512: 64eb110dad0ac67afbfbad506337ea1cea254c07d6739a71994bc0144a0fc4e5d35fa25ca588cea3a9d59a5f6164ca6cc28c41292da79d773127ea273450ac28

…rtup. 520d889 BugFix: g_IsSaplingActive flag wasn't being updated at startup. (furszy) Pull request description: Small bug introduced in #1814, the flag is set on every new tip but not initialized at startup. Discovered rebasing #1798 on top of master and running any sapling functional test. As `g_IsSaplingActive` is false at startup, the transaction hash is different, making the merkle tree hash different, failing in `VerifyDB`. ACKs for top commit: random-zebra: utACK 520d889 Fuzzbawls: utACK 520d889 Tree-SHA512: 609d0257a524aa85a12296ab827b0cd9dc640e60455801203d52709672e79cd9837f606743b976e5cc64d9f0659d81bd19b5bb6ec6b339fd01eebf44af58677e

5dda553 migrate list ser to the newer ser form. (furszy) Pull request description: Straightforward and easy to check PR. The list serialization wasn't updated to the latest serialization format (`nType` and `nVersion` removal). Decoupled from #1798 --> 4053e97 . ACKs for top commit: Fuzzbawls: ACK 5dda553 random-zebra: utACK 5dda553 and merging... Tree-SHA512: eddf0db7ac17aa6a56cc3a293a2bb0787a0771271cb8452191520869a8f9dd3226a351b2eb01b403d99d6b9d91e1094f4b620bb5a65e5bfd8c37ecbcc6303e85

furszy · 2020-10-03T02:20:48Z

Rebased on top of master, solved the conflicts and adapted the code to all new changes on master.

Linux travis job will continue failing in two unit test here for now, the solutions were directly included in #1870 and #1884 PRs. (1) json ser/deser, (2) boost filesystem error.

Once them get merged, will rebase it again.

626b0d9 incrementalmerkletree file added to cmake. (furszy) 4368a00 incremental merkle tree test: resolve invalid deserialization/serialization data equality check. (furszy) 9cfb0a7 rename sapling util file to not shadow src/util file. (furszy) 68d494d Incremental Merkle Tree + merkletree tests back port (furszy) Pull request description: This is coming from #1798, focused on include the incremental merkle tree primitive with all of its unit tests. This base primitive class is almost one to one with upstream. Decoupled the following commits: * Incremental Merkle Tree + merkletree tests back port —> 762d5f1 * Rename sapling util file to not shadow src/util file --> cae89d3 ACKs for top commit: random-zebra: ACK 626b0d9 Fuzzbawls: utACK 626b0d9 Tree-SHA512: 9bae913aa553ec42864b5f5f252f8ce6bd9fd6fd89011c63c31e1de06cba820dc99feeaabe38801f012a54abc81b8213aee0334695c4e0673591e48dc07d5954

b11b357 removing global namespace usage for noteencryption and sapling_note unit tests (furszy) 52da4e0 [Sapling] Note Encryption unit tests back ported. (furszy) 4d2f01a [Sapling] change cm() to cmu() in SaplingNote class (furszy) Pull request description: Another decoupling from #1798. Similar to #1870, these changes are part of the primitives unit test coverage back port work. Commits included: * change cm() to cmu() in SaplingNote class —> d437276 * Note Encryption unit tests back ported. —> e4c1bbf ACKs for top commit: Fuzzbawls: ACK b11b357 random-zebra: re-utACK b11b357 Tree-SHA512: 56914dba65ee239f7e6713a111abca5bd1a4e684fadcdbed4314295f499f3b6689271fa7999c7f182dcba069ebe29299bed944e7ee6b1bcb3aad987884811242

69db60e Update sighash tests to include Sapling transactions. (furszy) Pull request description: Decoupled from #1798 d0099f5 . Initial sighash unit test coverage for shielded transactions. ACKs for top commit: random-zebra: utACK 69db60e Fuzzbawls: ACK 69db60e Tree-SHA512: f75a9241d3f9fad1f9c6928e7926630dbc9e5d45c888732d45fa5bf481ac7a5e742142de6e62346cb7691bdda9705e90ecb09167fe2123c726c61abd74c1a82a

random-zebra

Really awesome stuff, left some more comments, but most of them can be tackled later.

ACK 50bcaa8

src/wallet/rpcwallet.cpp

random-zebra · 2020-11-04T15:28:08Z

src/test/librust/sapling_rpc_wallet_tests.cpp

+    // We shouldn't be able to decrypt with the empty ovk
+    BOOST_CHECK(!libzcash::AttemptSaplingOutDecryption(
+            tx.sapData->vShieldedOutput[0].outCiphertext,
+            uint256(),


nit: UINT256_ZERO

src/wallet/rpcwallet.cpp

test/functional/sapling_wallet.py

random-zebra · 2020-11-04T16:12:46Z

test/functional/sapling_wallet_anchorfork.py

+        self.num_nodes = 3
+        self.setup_clean_chain = True
+        saplingUpgrade = ['-nuparams=v5_dummy:1']
+        self.extra_args = [saplingUpgrade, saplingUpgrade, saplingUpgrade]


same style nit as in the other test

test/functional/sapling_wallet_anchorfork.py

random-zebra · 2020-11-04T16:18:12Z

test/functional/sapling_wallet_nullifiers.py

+        connect_nodes_bi(self.nodes, 0, 1)
+        connect_nodes_bi(self.nodes, 2, 1)
+        connect_nodes_bi(self.nodes, 3, 1)


why bidirectional connection?

that is a good question for the entire test suite after a node shutdown/restart actually.

test/functional/sapling_key_import_export.py

random-zebra · 2020-11-04T16:33:15Z

test/functional/sapling_key_import_export.py

+
+        verify_utxos(bob, amounts, bob_addr)
+        verify_utxos(charlie, amounts, ipk_addr["address"])
+        verify_utxos(charlie, amounts, ipk_addr2["address"])


no need to verify again, as we already ensured (line 106) that ipk_addr["address"] and ipk_addr2["address"] are equal.

RPC: Include shielded_sendmany non string params.

sapling_wallet.py test added to test_runner.py test suite: get_coinstake_address util method implemented.

… our sources.

Back port + adaptation to our sources.

…r sources

…il the functionality gets implemented.

@Fuzzbawls

Commit from @Fuzzbawls :) .

…tead of returning an error string.

furszy · 2020-11-04T18:32:43Z

Updated per feedback, left only few nit comments for another PR.
Ready to finally merge this one ☕ ☕ .

random-zebra

re-utACK 1b6f701 after last push

Fuzzbawls

ACK 1b6f701

9047f74 SSPKM: getCredit improved using the SaplingNote cached amount. (furszy) b8a6a16 Cache note amount in the SaplingNoteData to not have to decrypt the note every time that the wallet needs to know the note value. (furszy) 56684af CWalletTx: calculate + cache shielded available credit and shielded change unit test. (furszy) 4e072de CWalletTx, shielded available credit cache flow implemented. (furszy) 565b3b1 Cache shielded change in CWalletTx. (furszy) 3a87eb1 Sapling wallet: Implemented GetShieldedChange (furszy) 9934f23 Unit test to validate the cached balance over shielded to remote shielded transactions. (furszy) 0d34607 Unit test verifying the cached shielded debit & credit balances. (furszy) b47e783 Move GetCredit CTransaction to CWalletTx to be able to decrypt the sapling note if needed. (furszy) 29c8641 Implement and connect to wallet GetDebit for shielded amounts. (furszy) 8f19bc7 Implement GetCredit for shielded amounts and connect it to the wallet. (furszy) 00ece7e Include shielded type to ismine enum. (furszy) 6ef8da4 Implement IsNoteSaplingChange simpler wrapper method. (furszy) 1742fc9 Wallet: decouple FindNotesData and AddMissingIVKToKeystore code from AddToWalletIfInvolvingMe. (furszy) 66fa102 Create method GetValidSaplingReceive to receive shielded balance from dummy transparent inputs. (furszy) d660344 Wallet::AddToWalletIfInvolvingMe use blockHash instead of CBlockIndex pointer. The block index is not needed and it will make unit tests simpler to code. (furszy) 2996db3 Wallet::Setup, memory only flag to not try to write the chain data in db (only used in unit tests). (furszy) Pull request description: Up until now, one of the several capabilities that #1798 introduced was the wallet's shielded transactions balances calculation, a purely on-demand process (which is not so performant as txs balances are requested more than once during the entire software lifecycle). This PR is going further, introducing a new wallet balance cache system for shielded transactions. Covered by a good amount of units tests that can be run pre and post the new cache system. This PR is obviously built on top of 1798. Part of the shielded GUI work that will be coming soon. ACKs for top commit: random-zebra: utACK 9047f74 (with few nits that can be ignored). Fuzzbawls: utACK 9047f74 Tree-SHA512: 3fa8131b44f93e3d9c058fe745a73ed6fd14c4a045f807e5e7b48d4ba14c6255b893ee1293618732fdfd7c4c7e8fc365b282a12bb24b6891cadde0d1f0a9a9cf

furszy added Wallet Tests Network Sapling labels Aug 10, 2020

furszy self-assigned this Aug 10, 2020

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 15333b2 to 481e0b3 Compare August 11, 2020 13:31

2a5A1Ghu1 mentioned this pull request Aug 12, 2020

Release 3.0 theohmproject/ohmcoin#52

Merged

Fuzzbawls added this to the 5.0.0 milestone Aug 12, 2020

furszy mentioned this pull request Aug 20, 2020

Sapling transaction primitive data. #1814

Merged

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 955b8c6 to 561c5d3 Compare September 12, 2020 18:12

furszy mentioned this pull request Sep 24, 2020

BugFix: g_IsSaplingActive flag is not initialized at startup. #1867

Merged

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 561c5d3 to 6afe5f6 Compare September 24, 2020 03:18

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 6afe5f6 to 56ace43 Compare October 3, 2020 02:06

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch 3 times, most recently from 9260303 to 9fb808f Compare October 7, 2020 16:04

furszy mentioned this pull request Oct 7, 2020

[Sapling] Anchors and nullifiers integrated into the chain state view layered cache. #1903

Merged

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 9fb808f to 2c7502a Compare October 8, 2020 21:01

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 0903941 to 50bcaa8 Compare November 4, 2020 11:45

random-zebra previously approved these changes Nov 4, 2020

View reviewed changes

furszy and others added 15 commits November 4, 2020 14:49

RPC shielded_sendmany command implemented.

d5ff6aa

RPC: Include shielded_sendmany non string params.

Sapling operation and sendmany RPC tests

a5996a7

RPC listreceivedbyshieldedaddress back ported

3def31f

RPC: Back port viewshieldedtransaction.

3934193

RPC: back port getsaplingnotescount

5826262

[Wallet] SetMerkleBranch using uint256 blockHash instead of the pindex.

e95c192

test suite: full sapling wallet test suite :)

f23af46

sapling_wallet.py test added to test_runner.py test suite: get_coinstake_address util method implemented.

Functional test: sapling_wallet_anchorfork back ported and adapted to…

30d23ee

… our sources.

Functional test sapling_wallet_nullifiers.

9376966

Back port + adaptation to our sources.

Fully adapted sapling wallet_listreceived.py functional test.

99cd368

Sapling key import/export functional test back ported + adapted to ou…

9168d5a

…r sources

sapling_changeaddresses.py functional test back ported

bc9e726

TransactionRecord: prevent Sapling transactions from being parsed unt…

93f4052

…il the functionality gets implemented.

fixing cmake linux link order.

4cfd593

Commit from @Fuzzbawls :) .

CMake: moving sapling_core_write files to SAPLING_A library

e761e92

furszy dismissed random-zebra’s stale review via 1af57c0 November 4, 2020 18:25

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 50bcaa8 to 1af57c0 Compare November 4, 2020 18:25

[RPC] shielded_sendmany throwing an error if the operation failed ins…

1b6f701

…tead of returning an error string.

furszy force-pushed the 2020_v5_privacy_transactions_august_8 branch from 1af57c0 to 1b6f701 Compare November 4, 2020 18:29

random-zebra approved these changes Nov 4, 2020

View reviewed changes

Fuzzbawls approved these changes Nov 4, 2020

View reviewed changes

furszy merged commit 21fdc49 into PIVX-Project:master Nov 4, 2020

random-zebra mentioned this pull request Nov 5, 2020

[Core] Say "Goodbye" to SwiftTX #1945

Merged

Fuzzbawls mentioned this pull request Nov 5, 2020

[Test] Introducing tier two functional test suite + several fixes #1916

Merged

random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jan 3, 2021

furszy deleted the 2020_v5_privacy_transactions_august_8 branch November 29, 2022 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sapling Integration: Chain + Wallet transaction manager. #1798

Sapling Integration: Chain + Wallet transaction manager. #1798

furszy commented Aug 10, 2020 •

edited

Loading

furszy commented Oct 3, 2020

random-zebra left a comment

random-zebra Nov 4, 2020

random-zebra Nov 4, 2020

random-zebra Nov 4, 2020

furszy Nov 4, 2020

random-zebra Nov 4, 2020

furszy commented Nov 4, 2020

random-zebra left a comment

Fuzzbawls left a comment

Sapling Integration: Chain + Wallet transaction manager. #1798

Sapling Integration: Chain + Wallet transaction manager. #1798

Conversation

furszy commented Aug 10, 2020 • edited Loading

Sapling integration, milestone 3 and 4.

Sapling transaction data:

Sapling transactions management:

Blockchain:

RPC Server:

New Commands:

Modified Commands:

Unit test coverage:

Functional tests:

TODO, upcoming work:

Need further discussion:

furszy commented Oct 3, 2020

random-zebra left a comment

Choose a reason for hiding this comment

random-zebra Nov 4, 2020

Choose a reason for hiding this comment

random-zebra Nov 4, 2020

Choose a reason for hiding this comment

random-zebra Nov 4, 2020

Choose a reason for hiding this comment

furszy Nov 4, 2020

Choose a reason for hiding this comment

random-zebra Nov 4, 2020

Choose a reason for hiding this comment

furszy commented Nov 4, 2020

random-zebra left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

furszy commented Aug 10, 2020 •

edited

Loading

Sapling transactions management: 

Blockchain: 

RPC Server: 

Functional tests: 