Remove dummy confirmations in RPC API and GUI for InstantSend transactions #2040

gladcow · 2018-04-19T14:42:34Z

Remove instantsenddepth option and dummy confirmations for IX transactions.

UdjinM6

Very nice! 👍

See inline comments
+
There is no way to quickly distinguish between regular tx and IS in GUI now, you have to open tx details dialog to get some idea. Smth like 5164d34d17ff7078c90bebcf7814defa2c3bd6d6 + 4f1fd17e4d880246b79772b670ab134d3d2a7faa + 141c9b9d88e1bd30d379763dffb72d190037cbb4 should do it more or less ;)

PS. Pretty surprised that amount of changes is that small. I expected this transformation to be much more invasive originally tbh, glad that it's not the case :)

UdjinM6 · 2018-04-19T17:42:22Z

src/instantx.h

This comment shouldn't be deleted

UdjinM6 · 2018-04-19T18:14:37Z

src/wallet/wallet.cpp

Wouldn't IsTrusted() be enough here?

UdjinM6 · 2018-04-19T18:14:47Z

src/wallet/wallet.cpp

UdjinM6 · 2018-04-19T18:36:21Z

src/wallet/wallet.h

nit: change name to IsLockedByInstantSend or smth like that

UdjinM6

see inline comments

UdjinM6 · 2018-04-20T11:07:29Z

src/wallet/wallet.cpp

Err... That's not what I meant, I meant that addition of !pcoin->IsLockedIX() was excessive because it's already a part of IsTrusted(), not that smth else should be removed..

Actually... I double checked and I guess I was maybe wrong in my initial comment - we don't really check if tx is final or not when MN votes for some input so IsTrusted() == false && IsLockedIX() == true could be a thing probably. Pls revert this (and the one below) to your original version. Sorry 😇

EDIT: but we do check if lock request itself is final or not... 🙄 Anyway, more checks is probably better 😅

Yes, I've missed it too, will revert it now

UdjinM6 · 2018-04-20T11:07:38Z

src/wallet/wallet.cpp

UdjinM6 · 2018-04-22T14:04:06Z

src/wallet/wallet.cpp

"Fixed" the wrong one here, should not touch this and fix GetUnconfirmedWatchOnlyBalance() instead

UdjinM6

slightly tested ACK

UdjinM6 · 2018-04-23T10:19:28Z

Not really sure if we can include this in 12.3 or should we postpone this till 12.4 instead. On one hand, this only touches local wallet code and GUI/rpc, no consensus or stability critical parts are affected. On the other hand, don't want any surprises in wallet code this close to the release either and there are still some TODOs regarding icons etc. I'm leaning towards 12.4 for now unless someone voice strongly for 12.3 and is willing to draw/add icons etc.

codablock · 2018-04-23T11:14:14Z

utACK but would strongly prefer to put this into 12.4 or 12.3.x instead of 12.3.

nmarley

utACK, but I would also prefer to wait to merge until 12.4 or a 12.3.x patch release that can be well tested

PastaPastaPasta · 2018-06-26T12:27:53Z

Can someone explain why this change is important/good. I see no problem with displaying the dummy confirmations.

UdjinM6

Needs rebase

- bring back removed comment; - simplify 2 complex condition to ose `IsTrusted()` function only; - rename `IsLockedIX` function to 'IsLockedByInstantSend`;

UdjinM6

Tested again, looks good 👍

There is one small nit, see below. Plus we should probably update instantsend.md to match new logic/rpc param.

UdjinM6 · 2018-07-16T17:11:58Z

src/wallet/wallet.cpp

        {
            const CWalletTx* pcoin = &(*it).second;
-            if (pcoin->IsTrusted())
+            if(pcoin->IsTrusted())


nit: this line should have no changes (bring the whitespace back)

UdjinM6 · 2018-07-17T12:43:38Z

doc/instantsend.md

-The key thing to understand is that this value indicates the number of "confirmations" a successful Transaction Lock represents. When Wallet RPC commands which support `minconf` and `addlockconf` parameters (such as `listreceivedbyaddress`) are performed and `addlockconf` is `true`, then `instantsenddepth` attribute is taken into account when returning information about the transaction. In this case the value in `confirmations` field you see through RPC is showing the number of `"Blockchain Confirmations" + "InstantSend Depth"` (assuming the funds were sent via InstantSend).
-
-There is also a field named `instantlock` (that is present in commands such as `listsinceblock`). The value in this field indicates whether a given transaction is locked via InstantSend.
+There is a field named `instantlock` (that is present in commands such as `listsinceblock`). The value in this field indicates whether a given transaction is locked via InstantSend.


Looks pretty good. But there is no underlying mechanism anymore and Examples sub-section doesn't really give any additional valuable info now, so we probably can write the whole RPC section like this:

Details pertaining to an observed "Transaction Lock" can also be retrieved through RPC. There is a boolean field named instantlock which indicates whether a given transaction is locked via InstantSend. This field is present in the output of some wallet RPC commands e.g. listsinceblock, gettransaction etc. as well as in the output of some mempool RPC commands e.g. getmempoolentry and a couple of others like getrawmempool (for verbose=true only).

Thoughts? @codablock @nmarley

I agree, this paragraph seems more accurate (removes "underlying mechanism") and descriptive (adds info on boolean field and which RPC commands have it).

UdjinM6

ACK

- Update 8 RPCs based on dashpay/dash#2040

- Related to dashpay/dash#2040

- Update 8 RPCs based on dashpay/dash#2040

thephez mentioned this pull request Apr 19, 2018

Remove dummy confirmations in RPC API for InstantSend transactions dash-docs/dash-docs#63

Closed

8 tasks

gladcow force-pushed the ixopt branch from 41a7dfa to 9f52360 Compare April 19, 2018 21:24

UdjinM6 requested changes Apr 19, 2018

View reviewed changes

UdjinM6 reviewed Apr 20, 2018

View reviewed changes

UdjinM6 reviewed Apr 22, 2018

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

UdjinM6 Apr 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Fixed" the wrong one here, should not touch this and fix GetUnconfirmedWatchOnlyBalance() instead

gladcow force-pushed the ixopt branch from b3c10c1 to 685cc70 Compare April 22, 2018 18:18

UdjinM6 added this to the 12.4 milestone Apr 23, 2018

UdjinM6 approved these changes Apr 23, 2018

View reviewed changes

UdjinM6 requested review from codablock and nmarley April 23, 2018 10:20

nmarley previously approved these changes Apr 23, 2018

View reviewed changes

UdjinM6 mentioned this pull request May 4, 2018

Comment updates - InstantSend #2062

Merged

UdjinM6 requested changes Jul 15, 2018

View reviewed changes

gladcow dismissed nmarley’s stale review via 0c0722d July 16, 2018 10:25

gladcow force-pushed the ixopt branch from 685cc70 to 0c0722d Compare July 16, 2018 10:25

gladcow and others added 10 commits July 16, 2018 17:31

remove instandsenddepth option

a25c31f

remove CInstantSend::GetConfirmations

fd72346

Explicit IXlocks processing in the code using GetDepthInMainChain

8405a51

remove dummy confirmations for IX locks

9397743

remove unused constant

0a13728

Fix issues:

74c990e

- bring back removed comment; - simplify 2 complex condition to ose `IsTrusted()` function only; - rename `IsLockedIX` function to 'IsLockedByInstantSend`;

Track IS status in TransactionStatus

4732ed1

Highlight IS in tx list

64fa3eb

Customize confirmation icons for IS txes

4c08321

Revert condition with IsTrusted()

586114d

gladcow force-pushed the ixopt branch from 0c0722d to 586114d Compare July 16, 2018 14:31

UdjinM6 reviewed Jul 16, 2018

View reviewed changes

gladcow added 2 commits July 17, 2018 12:17

code style fix

2c04732

remove dummy confirmations from instantsend.md

101c400

UdjinM6 reviewed Jul 17, 2018

View reviewed changes

fix instantsend.md

eb73e1c

UdjinM6 approved these changes Jul 28, 2018

View reviewed changes

UdjinM6 merged commit 0a6f473 into dashpay:develop Jul 28, 2018

UdjinM6 mentioned this pull request Jul 28, 2018

Fix lockedByInstantSend initialization #2197

Merged

UdjinM6 mentioned this pull request Sep 1, 2018

Instantsend Bug? #2252

Closed

thephez added a commit to thephez/dash-docs that referenced this pull request Oct 22, 2018

Content - RPC - addlockconf to addlocked conversion

4e856ad

- Update 8 RPCs based on dashpay/dash#2040

thephez added a commit to dashpay/docs that referenced this pull request Oct 22, 2018

InstantSend argument and RPC updates for next release

9b5b9dd

- Related to dashpay/dash#2040

thephez mentioned this pull request Oct 22, 2018

InstantSend argument and RPC updates for next release dashpay/docs#16

Merged

thephez added a commit to dash-docs/dash-docs that referenced this pull request Nov 19, 2018

Content - RPC - addlockconf to addlocked conversion

c31012e

- Update 8 RPCs based on dashpay/dash#2040

Remove dummy confirmations in RPC API and GUI for InstantSend transactions #2040

Remove dummy confirmations in RPC API and GUI for InstantSend transactions #2040

Uh oh!

Conversation

gladcow commented Apr 19, 2018

Uh oh!

UdjinM6 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!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codablock commented Apr 23, 2018

Uh oh!

nmarley left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Jun 26, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 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!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

UdjinM6 Apr 20, 2018 •

edited

Loading

UdjinM6 commented Apr 23, 2018 •

edited

Loading