Sapling milestone 5: Graphical User Interface by furszy · Pull Request #1963 · PIVX-Project/PIVX

furszy · 2020-11-11T13:14:52Z

Essentially covering the last milestone to Sapling. Implementing the graphical user interface for the new privacy protocol: adding shielded transactions and addresses (own addresses and contacts) creation and management visual workflows, and shielded balances presentation and update.

Following points are covered (divided by areas):

Visual interface:

transaction record shielded types (shielded send, shielded receive, shielded send to self), shielded credit and debit amounts connected, shielded address decryption + cache.
dashboard transactions list and model modified to accept the new records types.
adding shielded balances to the model balances cache structure.
shielded balances connected to the topbar amounts.
bubble popup showing shielded and transparent amounts.
receive screen: shielded addresses generation, set/change label, copy and show wallet's shielded addresses list.
shielded addresses inputs validation.
contacts screen: creation, search and filter shielded addr contacts.
contacts dropdown showing shielded addrs.
send screen shielded spend implemented.
transaction creation moved to a worker thread.

Wallet:

IsMine for sapling addresses.
GetAvailableBalance filtered by shielded amounts.
GetUnconfirmedBalance filter for transparent and/or shielded.
new shielded outpoint GetDebit, GetCredit methods.
new GetAvailableShieldedBalance and GetUnconfirmedShieldedBalance.
decrypt only shielded addr from note.

Addressbook:

shielded addresses capabilities.
shielded receive and shielded send new purposes.
variant CWDestination generalization to be able to store shielded and regular addresses.

RPC:

getbalance added an includeShielded flag.

TODO:

shielded inputs/outputs and fee in the transaction/sendconfirm dialog.
notes coin control selection.
double check encrypted wallet tx scanning.
warning dialog for when the user is sending a transparent tx.
send confirmation dialog, fee isn't connected.
shielded txs and balances FAQ.

Extra note:
This is still under WIP tag, wording and visuals can change with the usage and feedback :) . Plus could have some issues that I'm currently fixing.

furszy · 2020-11-14T01:44:58Z

Updated per @NoobieDev12 (ambassador) initial testing 👌 . More are coming

ambassador000 · 2020-11-14T14:11:44Z

Needs rebase after 1964 merge.
(sent you a DM, but I guess you'll see it here first :P)

furszy · 2020-11-14T16:00:12Z

rebased, conflicts solved.

furszy · 2020-11-18T16:51:10Z

rebased on top of #1977 (needed for the coin control notes selection process) and solved conflicts with master's recently merged PRs.

…eanup and improvements 71e1f98 Wallet + GUI: IsChange function + coin control, setting change label for the change outputs (furszy) a463ac2 Model:ListCoins, abstracted returned value into a wrapper in order to be able to return unspent notes in the future as well. (furszy) 8ca9d45 GUI: coin control dialog, abstracting out row load. (furszy) 18e2761 GUI: coinControlDialog removing now unneded qt5.3 and qt5.4 workaround. Our minimum version is qt5.5 (furszy) fd19626 GUI: coinControlDialog removing unnecessary uncompressed keys size calculation. Our reference client is not creating them by default. Most likely never did it. (furszy) 3420701 GUI: coinControlDialog removing an totally redundant, unnecessary and badly coded function. (furszy) 30f67e0 wallet: moving ListLockedCoins to directly return the set instead of loop over it and then return it. (furszy) 9927b08 Wallet and GUI: remove unused tx priority calculation and send free transaction flag (furszy) f602fab CoinControlDialog::updateView removing redundant labelForAddress call. (furszy) 8aac899 CoinControlDialog::updateView removing never used dead code. The `sAddress == sWalletAddress` has been always true. (furszy) a19eb53 CoinControlDialog: making use of the new optional staker addresses that is retrieved by ListCoins (furszy) f2297a6 WalletModel::listCoins P2CS utxo grouped properly. (furszy) 6752f5b coin control, update view: removing unused priority calculation. (furszy) 031b006 Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0; (practicalswift) cf42757 [qt] coincontrol: Remove unused qt4 workaround (MarcoFalke) 100c006 coin control, update view method refactored pulling the output basic values to the top of the method. (furszy) a10cdcb walletModel: listCoins cleaning an overkill to get the address that originated the change (furszy) b65e670 walletModel: listCoins removing a unneeded ListLockedCoins call plus followup loop over the returned vector. (furszy) 3b9bf04 wallet::AvailableCoins introduced flag for including or not locked coins. (furszy) 831d10b wallet::AvailableCoins filter struct instead of having an endless amount of arguments in the function signature. (furszy) Pull request description: Base initial work in order to be able to generalize `CoinControlDialog` up to the point of introducing shielded notes selection (being developed in the Sapling GUI PR #1963). Found several redundancies and not so good code over the overall `CoinControlDialog` class. So.. have re-structured it in a large percentage, aiming to improve the performance and clean all of the inefficiencies. ACKs for top commit: random-zebra: Nice stuff 🥃 ACK 71e1f98 Fuzzbawls: ACK 71e1f98 Tree-SHA512: a905c987e832242d4c58c6dcc4ffba6a02d238da30180847475aa4fa5574d9e46730f397449f27be81005994147c4079ac5bb39da7b38845bfe5c0cf8c3205ab

random-zebra

Code review ACK till db3707f242e446ecfc92a0fc26a47b719bfc29bf.
Looking pretty good so far.
Left a couple talking points, mostly about ::IsMine, which could be much faster I think, and about the management of shielded watch-only addresses in getbalance.

furszy · 2020-11-21T15:35:59Z

Nice feedback :), will work on it.

random-zebra

Up to 01548f51844ae33f703e975a8d7a1763acaf0280...

random-zebra

Code review, and light-testing ACK dfdb0131fe79ca638af16156dc5803965e8f0c23.
Left some more comments.
Will do some more focused testing in the next days, but overall looks awesome. Great job 🥃

random-zebra · 2020-11-21T23:50:43Z

nit: in the future this could be refactored to remove the code duplication, adding a per-outpoint filter to SaplingScriptPubKeyMan::GetFilteredNotes

yeah, nice.

random-zebra · 2020-11-21T23:54:33Z

nit: unify this check and the one at lines 332-333, by putting it outside of if (hasCoinControl) {} else {}.

furszy · 2020-11-22T22:20:35Z

Awesome feedback zebra!, have started moving over all of the topics.

furszy · 2020-11-24T04:50:17Z

Updated per zebra's feedback (almost all of them tackled) plus pushed few more integrations (shielded tx fee visualization, tx dialog inputs view alignment fix and connected shielded txs & addresses filters in the dashboard and the addressbook export).

Thus making the GUI display a balance higher than it should.

furszy · 2020-11-28T21:20:54Z

Changes from @random-zebra merged, PR link, lot of awesome changes 🚀 .
Time to test this heavily and merge it if all is working fine.

random-zebra

Maybe worth keeping the shield category too, but including all possible related types?

    filter->addItem(QObject::tr("Shield"),
                    TransactionFilterProxy::TYPE(TransactionRecord::RecvWithShieldedAddress) |
                    TransactionFilterProxy::TYPE(TransactionRecord::SendToShielded) |
		    TransactionFilterProxy::TYPE(TransactionRecord::SendToSelfShieldToShieldChangeAddress) |
		    TransactionFilterProxy::TYPE(TransactionRecord::SendToSelfShieldToTransparent) |
                    TransactionFilterProxy::TYPE(TransactionRecord::SendToSelfShieldedAddress));

furszy · 2020-11-29T03:44:28Z

Pushed few more commits. All visual findings discovered testing the PR.

Fuzzbawls

Very nice work here! I've left some nit comments that i'd be ok with addressing in a follow-up PR as this is already getting massive. I've notices some consistency issues when starting the wallet with a mix of -reindex/-zapwallettxes=2, but I think it would be best to tackle those issues in subsequent PRs

Fuzzbawls · 2020-11-29T09:16:14Z

add notr="true"

Fuzzbawls · 2020-11-29T09:16:32Z

add notr="true"

Fuzzbawls · 2020-11-29T09:18:11Z

would change this to Transfer coins publicly or privately

Fuzzbawls · 2020-11-29T09:23:00Z

change to Select which coins to spend (more direct)

Fuzzbawls · 2020-11-29T09:25:00Z

add notr="true"

Fuzzbawls · 2020-11-29T09:57:57Z

spacing on this line, and styling on the previous line (brace should be on a new line of it's own)

Fuzzbawls · 2020-11-29T09:58:12Z

brace should be on a new line

Fuzzbawls · 2020-11-29T10:02:15Z

brace on new line

Fuzzbawls · 2020-11-29T11:02:35Z

brace on new line

ambassador000 · 2020-11-29T15:41:00Z

Testing with the latest changes from this branch.

For some UTXOs, After Fee amount is not shown correctly:

When copy-pasting the After Fee amount and trying to send a transaction, it is still returning Insufficient shielded funds...:

(1) Selecting an UTXO
(2) Copy-pasting the After Fee amount

(3) After pasting the After Fee amount:
a) Total remaining from the selected UTXO is > 0
b) For the Shielded send, remaining shows 0.23 tPIV transparent, instead of shielded:

…ed txs.

…ansactions plus setting the address receiving the shielded funds.

random-zebra · 2020-11-29T15:58:15Z

Testing with the latest changes from this branch.

@NoobieDev12 you are definitely sure to be testing the latest commit?
there were a couple forced pushes recently (and I think, at least some of these should be fixed).
If you do git log, you can see what's the HEAD of your branch.
If the head doesn't match with the one here, and you cannot pull, due to conflicts (for force-pushes) do this:

git reset --hard HEAD~10
git pull

random-zebra · 2020-11-29T16:58:12Z

Anyway... as Fuzz said, this has become a massive change-set, so am going to merge it now.
We can tackle remaining things in separate, focused, follow-up PRs.

random-zebra

ACK 64d3175

ambassador000 · 2020-11-29T17:46:48Z

@random-zebra Yeah, I'm still seeing these issues I mentioned above. I checked git log and it looks like it pulled all the recent commits. Anyways, I'll fresh-compile the latest master from scratch now since it is being merged to master and update you if issues are fixed.

1192272 [Cleanup] Remove CBaseOutPoint::GetHash() now unused (random-zebra) 1467074 [Refactoring] Index mapVotes by mnIds (outpoints) (random-zebra) 715c9ad [BUG] Fix CBudgetProposal/CFinalizedBudget::GetVotesHashes (random-zebra) 7420ef7 [RPC] fix vote hashes and masternode collateral outpoints (random-zebra) Pull request description: `BaseOutPoint` has a member variable named `hash`, but `GetHash()` does not return its value. It returns the hash of the serialized class (`hash` and `n`) instead. This is confusing and might lead to sneaky bugs (in fact, it already did in the past: #1963 (comment)) Further, the implementation of `GetHash()` is rather ugly, as it accesses the singleton pointer `&this->n` as an array (with `BEGIN()`/`END()`), which could result in misinterpretation or corruption of adjacent memory locations. This function is used only in two places: - in the RPC `getbudgetvotes` for the key `"nHash"`. This is wrong, as nHash is supposed to be the **vote** hash (as described in the help), and not the collateral outpoint hash. - to index `mapVotes` inside budgets and proposal objects. Here we can just index directly by collateral outpoint (instead of its hash). With these two changes, we can get rid of `BaseOutPoint::GetHash`. This also fixes another bug fund along the way: `CBP/CFB::GetVotesHashes` should return the vote hashes, not the masternode collateral hash. Note: `mapVotes` is not directly sent on the network, it is only stored locally for each proposal. ACKs for top commit: furszy: Code review ACK 1192272. Fuzzbawls: utACK 1192272 Tree-SHA512: 17c1b2644ebd26ece903fc5c0ae522bd24a38ee828511b01ba0ffbb27e3871906ee2ddc2248599e019f4d44d33b36219916e067bb9217a96998b02cec56a2340

furszy self-assigned this Nov 11, 2020

furszy changed the title ~~Sapling milestone 5: Graphical User Interface~~ [WIP] Sapling milestone 5: Graphical User Interface Nov 11, 2020

furszy added GUI Sapling labels Nov 11, 2020

random-zebra added this to the 5.0.0 milestone Nov 11, 2020

furszy force-pushed the 2020_v5_privacy_GUI branch 4 times, most recently from 7f57aa8 to a175b54 Compare November 13, 2020 02:41

furszy changed the title ~~[WIP] Sapling milestone 5: Graphical User Interface~~ Sapling milestone 5: Graphical User Interface Nov 13, 2020

furszy force-pushed the 2020_v5_privacy_GUI branch from 2dfc02f to 6f338a4 Compare November 14, 2020 14:57

furszy mentioned this pull request Nov 17, 2020

Coin control dialog utxo processing/update flow large cleanup and improvements #1977

Merged

furszy force-pushed the 2020_v5_privacy_GUI branch 2 times, most recently from 2afbbf3 to 9648dc7 Compare November 18, 2020 16:49

furszy force-pushed the 2020_v5_privacy_GUI branch from 75b191c to 05483e4 Compare November 20, 2020 22:39

random-zebra reviewed Nov 21, 2020

View reviewed changes

Comment thread src/sapling/saplingscriptpubkeyman.cpp Outdated

Comment thread src/sapling/saplingscriptpubkeyman.cpp Outdated

Comment thread src/qt/transactionrecord.cpp Outdated

Comment thread src/wallet/rpcwallet.cpp Outdated

Comment thread src/script/standard.h Outdated

furszy force-pushed the 2020_v5_privacy_GUI branch from 05483e4 to 798ccb0 Compare November 21, 2020 15:34

furszy force-pushed the 2020_v5_privacy_GUI branch from 798ccb0 to dfdb013 Compare November 21, 2020 15:39

random-zebra reviewed Nov 21, 2020

View reviewed changes

furszy force-pushed the 2020_v5_privacy_GUI branch from dfdb013 to f8fbf61 Compare November 22, 2020 21:50

furszy force-pushed the 2020_v5_privacy_GUI branch from f8fbf61 to 5b546aa Compare November 23, 2020 14:32

random-zebra added 5 commits November 28, 2020 17:44

GUI: Add SPORK_20 check in sendCoins

c26c65d

BUG: fix correct fee/amount in decomposeShieldedDebitTransaction

6b458ae

BUG: fix SSPKM::GetCredit counting externally sent notes

d2faa04

Thus making the GUI display a balance higher than it should.

GUI: connect labels calculation in coin control for shielded outputs

43c1e4b

Refactor: don't save empty memos in SaplingNoteData

9737bcf

random-zebra reviewed Nov 29, 2020

View reviewed changes

Comment thread src/qt/pivx/qtutils.cpp Outdated

furszy force-pushed the 2020_v5_privacy_GUI branch 2 times, most recently from f63065e to bc672d4 Compare November 29, 2020 00:37

random-zebra force-pushed the 2020_v5_privacy_GUI branch from 8ba335a to 78dda88 Compare November 29, 2020 01:04

Fuzzbawls previously approved these changes Nov 29, 2020

View reviewed changes

random-zebra and others added 7 commits November 29, 2020 12:54

GUI: CoinControlDialog::updateLabels -Loop over OutPointWrapper directly

033ea0c

Cleanup: remove unused walletModel::getOutputs

d5e97d4

GUI: send screen, hide custom change address for shielded transactions

8c946eb

GUI: dashboard, better shielded records filters.

fb12325

GUI: transaction detail/confirm dialog, fixing inputs grid for shield…

48c4f86

…ed txs.

GUI: transaction record, fixing shielded credit for shield to self tr…

0e0d3ad

…ansactions plus setting the address receiving the shielded funds.

GUI: send screen, simple texts changes.

64d3175

furszy dismissed Fuzzbawls’s stale review via 64d3175 November 29, 2020 15:58

furszy force-pushed the 2020_v5_privacy_GUI branch from e5b8434 to 64d3175 Compare November 29, 2020 15:58

random-zebra approved these changes Nov 29, 2020

View reviewed changes

random-zebra merged commit 94c182e into PIVX-Project:master Nov 29, 2020

random-zebra mentioned this pull request Feb 2, 2021

[Refactoring] Remove BaseOutPoint::GetHash #2180

Merged

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

Conversation

furszy commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Visual interface:

Wallet:

Addressbook:

RPC:

Uh oh!

furszy commented Nov 14, 2020

Uh oh!

ambassador000 commented Nov 14, 2020

Uh oh!

furszy commented Nov 14, 2020

Uh oh!

furszy commented Nov 18, 2020

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Nov 21, 2020

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy commented Nov 22, 2020

Uh oh!

furszy commented Nov 24, 2020

Uh oh!

furszy commented Nov 28, 2020

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Nov 29, 2020

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

furszy commented Nov 11, 2020 •

edited

Loading

Fuzzbawls Nov 29, 2020 •

edited

Loading