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

GUI update not always triggered #301

Closed
dexX7 opened this Issue Dec 3, 2015 · 20 comments

Comments

Projects
None yet
2 participants
@dexX7
Copy link
Member

dexX7 commented Dec 3, 2015

Some SP related transactions currently don't show up in the GUI, even after a confirmation. However, they appear, if the client is restarted, or when sending a simple send.

Here is an example:

  1. Start clean in regtest mode
  2. Generate new address: muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q
  3. setgenerate true 101
  4. Send some BTC to muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q
  5. setgenerate true 1
  6. omni_sendissuancefixed "muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q" 2 1 0 "Companies" "Bitcoin Mining" "Quantum Miner" "" "" "1000000" -- doesn't show up as unconfrimed
  7. setgenerate true 1 -- now the Omni transaction shows up
  8. omni_sendissuancecrowdsale "muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q" 2 1 0 "Companies" "Bitcoin Mining" "Quantum Miner" "" "" 2 "100" 1483228800 30 2 -- doesn't show up as unconfirmed
  9. setgenerate true 1 -- crowdsale not showing up
  10. omni_sendissuancemanaged "muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q" 2 1 0 "Companies" "Bitcoin Mining" "Quantum Miner" "" "" -- doesn't show up as unconfirmed
  11. setgenerate true 1 -- crowdsale and managed issuance not showing up
  12. omni_send "muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q" "muFUyodercdeo6JAgtJaStM88rMW9Jxo9Q" 2147483651 "10" -- all transactions show up
@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 4, 2015

This is going to be down to wallet caching I'd bet (since updates are triggered by changes to wallet balances) - as it's a new property the balance probably isn't being tracked by the wallet cache and thus when the question of "is there anything to update?" comes up, the wallet cache thinks "I don't see anything different, no need to update".

Taking a look now...

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 4, 2015

Also to note the lack of showing up as unconfirmed is because very few of our send RPC calls add a pending object (as you noted in another thread).

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 4, 2015

"I don't see anything different, no need to update"

Ah, this makes a lot of sense in this context:

  • fixed issuances show up, and tokens are created
  • crowdsales or managed property creations don't show up, and no tokens are created
@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

I repeated your test with debugging enabled for the wallet cache, and indeed that's where the problem is...

So we create an address & do our send fixed issuance and mine a block, all appears fine - the wallet cache detects the new address and notifies that we need an update. Note however that the cache is not detecting the new fixed issued tokens, it's calling for an update because it detected a new address in the wallet:

2015-12-04 23:52:39 parseTransaction(block=103, 2015-12-04 23:52:39 idx= 1); txid: d2d50a9f83f8027115c83487c995c937c2fcd3b26cdd02062a01f34e1678159f
2015-12-04 23:52:39     ------------------------------
2015-12-04 23:52:39              version: 0, class B
2015-12-04 23:52:39                 type: 50 (Create Property - Fixed)
{...}
2015-12-04 23:52:39 update_tally_map(mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL, 2147483651=0x80000003, +1000000, ttype=0): before=0, after=1000000
2015-12-04 23:52:39 recordTX(d2d50a9f83f8027115c83487c995c937c2fcd3b26cdd02062a01f34e1678159f, valid=YES, block= 103, type= 50, value= 1000000)
2015-12-04 23:52:39 WALLETCACHE: Update requested
2015-12-04 23:52:39 WALLETCACHE: *CACHE MISS* - mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL not in cache
2015-12-04 23:52:39 WALLETCACHE: Ignoring non-wallet address mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv
2015-12-04 23:52:39 WALLETCACHE: Update finished - there were 1 changes

Then we go and do our crowdsale:

2015-12-04 23:53:06 ____________________________________________________________________________________________________________________________________
2015-12-04 23:53:06 parseTransaction(block=104, 2015-12-04 23:53:06 idx= 1); txid: f5753fed3456553dc45de52b071ca0cd8f106c516ae152c79fcdb5fca892d294
2015-12-04 23:53:06     ------------------------------
2015-12-04 23:53:06              version: 0, class B
2015-12-04 23:53:06                 type: 51 (Create Property - Variable)
{...}
2015-12-04 23:53:06 CREATED CROWDSALE id: 2147483652 value: 100 property: 2
2015-12-04 23:53:06 recordTX(f5753fed3456553dc45de52b071ca0cd8f106c516ae152c79fcdb5fca892d294, valid=YES, block= 104, type= 51, value= 100)
2015-12-04 23:53:06 WALLETCACHE: Update requested
2015-12-04 23:53:06 WALLETCACHE: Ignoring non-wallet address mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv
2015-12-04 23:53:06 WALLETCACHE: Update finished - there were 0 changes

The wallet cache detects no changes, and the UI is thus not updated.

Exactly same thing for the managed issuance, no changes detected:

2015-12-04 23:53:32 ____________________________________________________________________________________________________________________________________
2015-12-04 23:53:32 parseTransaction(block=105, 2015-12-04 23:53:32 idx= 1); txid: 53a26158b3f71dca46bcb004a67048f7aedb8ba1301832ce796776efb3012956
2015-12-04 23:53:32     ------------------------------
2015-12-04 23:53:32              version: 0, class B
2015-12-04 23:53:32                 type: 54 (Create Property - Manual)
{...}
2015-12-04 23:53:32 CREATED MANUAL PROPERTY id: 2147483653 admin: mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL
2015-12-04 23:53:32 recordTX(53a26158b3f71dca46bcb004a67048f7aedb8ba1301832ce796776efb3012956, valid=YES, block= 105, type= 54, value= 0)
2015-12-04 23:53:32 WALLETCACHE: Update requested
2015-12-04 23:53:32 WALLETCACHE: Ignoring non-wallet address mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv
2015-12-04 23:53:32 WALLETCACHE: Update finished - there were 0 changes

Then finally we come to the send, which sets up a pending object and the balance change is detected, triggering an update:

2015-12-04 23:53:50 PendingAdd(ec39e685200914142a7f2c36f9d430fe0739a987b33fd556115078f1b5135d22,mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL,0,2147483651,10,true)
2015-12-04 23:53:50 update_tally_map(mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL, 2147483651=0x80000003, -10, ttype=3): before=0, after=-10
2015-12-04 23:53:50 WALLETCACHE: Update requested
2015-12-04 23:53:50 WALLETCACHE: *CACHE MISS* - mmRmgZS88w17qkvAb53WXdhqEA8UPnjJrL balance for property 2147483651 differs
2015-12-04 23:53:50 WALLETCACHE: Ignoring non-wallet address mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv
2015-12-04 23:53:50 WALLETCACHE: Update finished - there were 1 changes

So that's definitely the issue I think, and I think it relates to any creation - looking into it further now :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

A hacky solution could be to:

     // transactions were found in the block, signal the UI accordingly
-    if (countMP > 0) CheckWalletUpdate();
+    if (countMP > 0) CheckWalletUpdate(true);

In the block handler, to force an update, if there were Omni transactions, even if they may not always involve the wallet.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

A hacky solution could be to...

Well it's interesting you should say that, because these days I do kind of question the need for the wallet cache entirely. It's original purpose was to drastically reduce the number of updates we were placing in the message queue during parsing by making sure we only updated on relevant changes - basically to try and arrest the message queue growing huge and locking up the UI.

You've already implemented a more robust solution by dropping new UI update requests when there are already UI updates waiting, which unless I've forgotten another use for it, kind of removes the need for the wallet cache.

While we're on the topic though I'm not sure why it's not working, the current tally (which contains the new tokens) is compared against a cached copy of the tally (which doesn't contain the new tokens) which should result in comparing some positive number to a zero and thus detecting a change, but it doesn't trigger

Anyway, perhaps it's time to test getting rid of the cache altogether and seeing how the UI handles (your suggested change would work fine to test this).

EDIT: nevermind, was having a slow brain moment - of course you were correct (crowdsales and managed don't immediately create tokens) - sorry completely missed that - yeah this all makes sense now.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

Offtopic - are you clear on release? I'm kind of grey on it - I think it's Mon/Tue, so does that mean we'll be releasing branch as current?

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

... which should result in comparing some positive number to a zero and thus detecting a change, but it doesn't trigger:

These transaction types don't update the tally at all (and an attempt would be rejected, given that we currently don't allow zero money updates).

Anyway, perhaps it's time to test getting rid of the cache altogether and seeing how the UI handles (your suggested change would work fine to test this).

Well, in general I'd say: let's not make the situation worse, dropping UI events kinda works, but ideally we fire only, if needed. Then again, if the impact is not really notable, then we could simplify and slim down things a bit, which is probably good. :)

Offtopic - are you clear on release?

Yeah, even though there were a few bug reports in the last days (like this one), I wouldn't want to delay any further, and if there are outstanding changes, then I'd like to keep them minimal.

My plan was to push the version bump tomorrow (Saturday), so we have sufficient time for the building.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

OK sounds good - also there is nothing to stop us doing a 0.0.10.1 soon after with some more bugfixes as long as the consensus critical code remains unchanged we're fine :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

You're right, and I like this attitude! :)

I tested the quick fix from #301 (comment), and it seems to work. But I'd also be fine to postpone a fix.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

... which should result in comparing some positive number to a zero and thus detecting a change, but it doesn't trigger:

Just some brainstorming for some point in the future: actually it would be nice, if we could handle "zero balances" in this context, or let me rephrase, it would be nice, if, say for example, after creating a managed property, a new entry in the balance tab is shown with the new property, but zero balance.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

OK cool, testing #301 now...

P.S. finally got to the bottom of the missing DEx sells on new version of OmniChest after a few reparses hehe - it wasn't the case change "New" to "new" in the end, it was that we changed the attribute name (subaction > action) which I forgot about & it was tripping up the engine knowing where to record the sell.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

P.S. finally got to the bottom of the missing DEx sells

Ahh, that's nice to hear! :)

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

Sorry, testing #303 not #301 - and it all works as expected... OK to merge :)

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

it would be nice, if, say for example, after creating a managed property, a new entry in the balance tab is shown with the new property, but zero balance.

I think all we need to do is get the property ID into the tally for that address. I know we're not allowed to do zero value tally updates but we could have another function that is explicitly for adding a zero balance property to the tally for a given address, which could be called from interpretation code.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 5, 2015

It's probably a bit more tricky, because we assumed until now zero balances should be completely ignored, e.g. via #262 (Filter empty balances in omni_getall* RPC calls), which is pretty related to this.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 5, 2015

Ahh I see, so when the client is restarted for example, their zero balance entry in the balance tab disappears - yeah probably needs a little more thought :)

@dexX7 dexX7 closed this in #303 Dec 5, 2015

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 7, 2015

I just noticed: while valid transactions show up in the transaction history after a confirmation (or earlier, for those with full unconfirmed support); invalid transactions are currently only shown, if a valid transaction triggers the update.

This is because we currently only trigger a refresh, if there are valid Omni transactions.

@dexX7 dexX7 reopened this Dec 7, 2015

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 7, 2015

This is because we currently only trigger a refresh, if there are valid Omni transactions.

Perhaps this has gone awry at some point then - the block handlers were supposed to provide the count of Omni transactions in the block (not the count of valid Omni transactions in the block) - I'll take a nose around :)

@dexX7 dexX7 modified the milestone: 0.0.10.1 Dec 16, 2015

@dexX7 dexX7 modified the milestone: 0.0.10.1 May 17, 2016

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Sep 19, 2017

This issue is probably resolved.

@dexX7 dexX7 closed this Sep 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment