Skip to content
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

[10] UI calls get_full_accounts for a lot more accounts than are in my wallet #202

Closed
destenson opened this issue Jul 22, 2017 · 22 comments
Closed
Assignees
Milestone

Comments

@destenson
Copy link
Contributor

@destenson destenson commented Jul 22, 2017

... that make the bitshares platform frustrating to use and often a significant waste of time.

I would love to begin fixing many of the bugs that plague me and others (mostly in the gui, and some in the native code too), but I cannot get it to build either on linux or windows, in docker or vagrant or otherwise. Can someone who can help me take a little time & help me get my clones into a state where I can begin to make contributions? The last time I had it building was over a year ago, and that was 2 computers ago, so I dont' have the same environment to go back to.

I feel like I waste a lot of time clicking around trying to find potential trades, which usually aren't there (but sometimes are), or waiting for this:

!!! GrapheneApi error:  get_full_accounts (2) [Array(1), true] Object {code: 1, message: "Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: ", data: Object}code: 1data: Objectmessage: "Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: "__proto__: Object {"code":1,"message":"Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: ","data":{"code":10,"name":"assert_exception","message":"Assert Exception","stack":[{"context":{"level":"error","file":"database_api.cpp","line":621,"method":"get_full_accounts","hostname":"","thread_name":"th_a","timestamp":"2017-07-22T03:03:45"},"format":"std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: ","data":{}},{"context":{"level":"warn","file":"websocket_api.cpp","line":122,"method":"on_message","hostname":"","thread_name":"th_a","timestamp":"2017-07-22T03:03:45"},"format":"","data":{"call.method":"call","call.params":[2,"get_full_accounts",[["1.2.xxxxxx"],true]]}}]}}
20:03:45.816 app.js?3381fa9…:1 Error:  Object {code: 1, message: "Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: ", data: Object}code: 1data: Objectmessage: "Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: "__proto__: Object

error to stop, that never does. I'd rather try to waste that time more productively by attempting to fix some of these issues and make it a better platform that more people want to use (including myself).

That one's especially annoying since it can cause the entire GUI to fail to load. Really, there's very little reason that the GUI needs to get_full_accounts on every single account it knows about... The signal to noise ratio in that call is extremely low since very little of the information provided by it when it succeeds seem to be remotely necessary, even for main accounts, let alone one-offs that are still in my wallet that I can't remove and cause my user experience to go down the toilet... (not to mention the network and processor load on the server to have to supply all this frivolous information when the GUI asks for it upon login, and then repeatedly every 5 seconds until logoff.)

Also, do you guys realize that every time you click a link or load a page, it downloads the same 5MB bundle of code? I've looked through it, and a lot of it can be consolidated or eliminated. Sure people have fast connections now, but 5 megabytes per refresh, to download the same javascript bundle that rarely changes? And it's almost entirely compressible text, yet it's sent uncompressed.

Anyway, back to my point. Would someone, experienced with configuring the build environment for bitshares-core and -ui, please help me out so I can try & help you guys out. I have 20+ years of C/C++/C#.NET development experience, the last 10 of those professionally for a fortune 500 company, and I've spent most of my spare time in the last two years learning cryptocurrencies & blockchain technologies, and development with javascript. I think I can help out a little here, if someone else can help me get the damn thing building.

Thanks

@landry314

This comment has been minimized.

Copy link

@landry314 landry314 commented Jul 22, 2017

yes, please help this man help save us all!

the gui needs so much work. the core is such incredible code it is a shame to have a basically BETA gui still running on top of it!

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Jul 23, 2017

@destenson You comments are appreciated. Do you have React chops?

@destenson

This comment has been minimized.

Copy link
Contributor Author

@destenson destenson commented Jul 25, 2017

@wmbutler, I am no expert at React, but I've dabbled for about a year, and always willing to learn more. I intend to make minor modifications at first. Hopefully, then make more significant contributions as time goes on. Do you know who I can talk to, to get my development environment up to snuff so I can start looking at some of this stuff?

@landry314, That's my impression too. Much of the lower level code appears to be quite brilliantly crafted, but unfortunately isn't the the case throughout the stack. I don't know if I'll help "save us all", but I'll do my best. UI is not my strong suit, but fortunately, bug-fixing is. And with the GUI already in place, I shouldn't have a problem cleaning some of it up behind the facade. (and I'm seeing a lot of dirty corners back there)

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Jul 26, 2017

I'm going to distill this into one issue, your claim that 5MB is downloaded on every page load. Other than that, a general venting does nothing to solve indicual items. I would encourage you to:

  1. First search issues to see if your particular concern has been booked
  2. Post each issue as a separate issue with finite reproducible steps

This will allow us to prioritize and fix items. Thanks for your help and concern!

@destenson

This comment has been minimized.

Copy link
Contributor Author

@destenson destenson commented Jul 26, 2017

@landry314

This comment has been minimized.

Copy link

@landry314 landry314 commented Jul 26, 2017

he is trying to help develop, make real contributions in code, not just point out bugs and suggest features. i never saw it as venting, i saw it as an offer of help, from the very first comment.

please help him build the native code!

@svk31

This comment has been minimized.

Copy link
Contributor

@svk31 svk31 commented Jul 30, 2017

It actually is necessary for the GUI to use get_full_accounts in order to stay synced with the blockchain, so the real issue here is that you're using a wallet with tons of accounts, something the GUI was never designed to handle.

I'm not sure what your building issues are, it works fine for me on multiple computers, although I never tried in Windows. When you say native code are you referring to bitshares-core? If so, please make an issue in that repo instead.

@destenson

This comment has been minimized.

Copy link
Contributor Author

@destenson destenson commented Jul 31, 2017

Hi Sigve, it's been over a year since I've worked on BitShares's code. I'm not sure if you remember me, but I had a few PRs approved in the spring of last year. At that time I was still pretty new with Javascript development and especially React, so my contributions weren't very substantial.. Anyway some issues have been bothering me for quite a while and since I recently quit my employer of the last 10 years, now I've got a some more time to look.

When I opened this issue 9 days ago, i actually meant for it to be on the bitshares-core repository, but I left it here since the issues affect the GUI as well in the hopes that I could get a dialog started and get the full stack building and debuggable for me.. But since my build issues are now only preventing me from working on bitshares-core, I'll open the issue there.. I have not been able to successfully build and run it on either Windows or LInux. After much strife, I did get it to build on linux a few months ago, but it crashed almost immediately and I didn't have time to pursue it any further.

Now I have more time, and I'd love to help out & make some improvements wherever I can. Now I've gotten the UI building and running properly and have looked into the UI code more closely. There are a number of things I've found that I hope to improve in the coming weeks. On thing that stood out to me immediately is that state changes in react components are almost always being done incorrectly in the components I've examined. But, I could be wrong about that one, since I'm not familiar with the alt library it uses to assist with state transitions.

Regarding "the real issue", as you called it:
image
as of this morning, I can confirm that it:is not exclusively occurring only in a wallets with "tons of accounts". I have removed all my wallets from my browser and currently have a single wallet loaded with 2 accounts and 3 private keys. There are no other wallets in my browser's cache or local storage.. And, for the first few days, the performance was quite improved, and is generally much better, until this error spam occurs..

For whatever reason, the UI calls get_full_accounts for a lot more accounts than are in my wallet and causes assertions to fail on the server, in websocket_api.cpp: Assert Exception: std::distance(_subscribed_accounts.begin(), _subscribed_accounts.end()) <= 100: ", data: Object}... At the very least, the UI should notice these calls are failing and stop trying at some point. Ideally it should only call them when absolutely necessary, and the server should not be making this assertion. I suspect the problem is more widespread than you think, and may be underlying or contributing to several other issues I've read about that seem to be unrelated. I bet if more people opened their console when experiencing issues they'd notice it seems to be related too.

And this is why I want to get the full stack running for me, so I can trace the cause of the problem back to wherever it's occurring, and/or help think of fixes or workarounds so that it doesn't continue to be a problem for me and others. Personally, I think step one is get rid of the failing assertion on the server & see what happens..

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 4, 2017

Unfortunately, I had no problems starting the bitshare-ui project on my Mac. Maybe you could open a separate issue about building and I could see the errors you are getting during your npm install step...

Definitely placing this in an early Sprint to evaluate. We have a Worker Proposal for dealing with UI issues active. Please vote for it if you'd like to see progress in this area.

@wmbutler wmbutler added this to the 170914 milestone Aug 4, 2017
@wmbutler wmbutler changed the title Numerous loose ends, inconsistencies and utter annoyances.... [4] UI calls get_full_accounts for a lot more accounts than are in my wallet Aug 4, 2017
@wmbutler wmbutler modified the milestones: 170901, 170914 Aug 4, 2017
@destenson

This comment has been minimized.

Copy link
Contributor Author

@destenson destenson commented Aug 4, 2017

I am also not having a problem running the UI anymore. I still can't get bitshares-core working, but I'll open an issue on that repo when I decide to tackle it again.

After tracking down the assertion on the native side (and finding it's being reported away from where the problem is occurring), I noticed that the assertion only occurs when you try to auto-subscribe too many accounts and that get_full_accounts has an argument to tell it whether you want it to auto-subscribe or not. Then I found in bitsharesjs that it is being hardcoded to auto-subscribe. I've made a preliminary change there to stop hard-coding that parameter. I've submitted a PR #bitshares/bitsharesjs#3.

Also in that library, the code keeps attempting to get_full_accounts on all unsubscribed accounts every 5 seconds until the calll succeeds for them all, which can be never for me and others with several accounts. I haven't made a change to fix this, but I did get my account into a state where it was destroying performance & making the UI almost unusable. While its was in this state, I changed it to call get_full_accounts without auto-subscribe enabled. This resulted in the rest of the accounts being loaded, and performance returning to full reactivity...

I think it would be a good change to that code for it to do automatically what I did manually, and detect the failing API calls & stop trying to auto-subscribe when it does. I would also like to see this change done for the "ignored" accounts in the wallet. Between those two changes, I think several of my issues regarding performance & many account wallets would be fixed.

Although, I still have no idea why I've seen the console log tell me it's loaded as many as 1860 accounts when my wallet has 2 accounts and 3 private keys. Unless maybe it's loading markets and reporting them as accounts, and loading traders' accounts from those markets?
image

Also, I didn't realize you guys have been hired to maintain this. I don't mean to step on any toes. Just trying to help figure out some of these problems too. I noticed that the unit tests don't seem to be very thorough, and many are just stubs and don't test anything at all. So, I've started implementing more of these this week. (And learning a lot about Mocha and the way bitsharesjs & bitsharesjs-ws work.)

@calvinfroedge

This comment has been minimized.

Copy link
Contributor

@calvinfroedge calvinfroedge commented Aug 15, 2017

@destenson @wmbutler Can we get simplified / condensed / step by step reproduction steps for this issue?

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 15, 2017

@calvinfroedge I think it would be wise to increase the hourly value for this and claim it and research it. There is a lot of detail here and the fix may involve the creation of a better api call from bitshares-core in cooperation with @oxarbitrage. This is a good project for you to research personally.

@wmbutler wmbutler changed the title [4] UI calls get_full_accounts for a lot more accounts than are in my wallet [10] UI calls get_full_accounts for a lot more accounts than are in my wallet Aug 15, 2017
@destenson

This comment has been minimized.

Copy link
Contributor Author

@destenson destenson commented Aug 16, 2017

@calvinfroedge, I'd be glad to help with what I can. Which particular issue are you referring to? I've found that most of my problems stem from having "too many" accounts in my wallet. I'd be glad to share one particularly troublesome wallet, if there is a way to remove the password and some especially confidential accounts from it.

@wmbutler, I don't think the API from bitshares-core needs changing for this issue. The bitsharesjs consumer of that API needs to use it appropriately and not have hard-coded arguments. Please take a look at bitshares/bitsharesjs#3 for more information regarding this.

@calvinfroedge

This comment has been minimized.

Copy link
Contributor

@calvinfroedge calvinfroedge commented Aug 16, 2017

@destenson What I need to do in order to be able to resolve the issue is essentially start from a fresh wallet / account, and follow a deterministic series of actions until the issue occurs. That way I can see at what point the issue actually starts occurring and why.

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 16, 2017

@calvinfroedge I have a lot of accounts in my wallet. Want to do a screen share? Oh, and you are planning to claim this one right?

@wmbutler wmbutler added this to Backlog in Current Sprint Aug 17, 2017
@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 21, 2017

@svk31 @calvinfroedge Since this is a 10-hour issue and since we have just 1 more week in the Sprint, I need to know whether you are choosing to tackle this or not. If it's happening, please claim it and let's start discussing fixes.

@abitmore

This comment has been minimized.

Copy link
Member

@abitmore abitmore commented Aug 22, 2017

@wmbutler: as @destenson mentioned above, please check bitshares/bitsharesjs#3 . IMHO it's almost fixed already. Also recently we pushed a patch to the core: bitshares/bitshares-core#364 . More info in bitshares/bitshares-core#295

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 22, 2017

This is great news. Will we require any UI changes that our team might currently be unaware of in order to fix the problem in the front end, or will the js changes take effect without additional modifications?

Also, what is the anticipated delivery date for the fix. Will it be in place prior to Sept 1, 2017?

@wmbutler

This comment has been minimized.

Copy link
Contributor

@wmbutler wmbutler commented Aug 22, 2017

@svk31 will need for you to help determine whether or not any UI changes are necessary as part of the overall solution to this problem. This issue needs to be claimed and estimated if applicable, or closed and set to zero hours. It's one of the last ones for the 120901 Milestone. Thanks.

@svk31

This comment has been minimized.

Copy link
Contributor

@svk31 svk31 commented Aug 22, 2017

It will require GUI changes, and it's not quite as simple as abit and destenson think it is. The GUI is heavily dependent on get_full_accounts in order to have an up to date state of the blockchain, so in addition to changes proposed by destenson I will also need to make changes to the ChainStore, and make an audit of the GUI code to see where it will indeed be possible to not fetch data using get_full_accounts. I already tried this once back when these issues first surfaced and it's not that straight forward.

@abitmore

This comment has been minimized.

Copy link
Member

@abitmore abitmore commented Aug 22, 2017

@svk31 you mean have to use get_full_accounts with subscription or can call it without subscription?

@svk31

This comment has been minimized.

Copy link
Contributor

@svk31 svk31 commented Aug 22, 2017

I mean the GUI requires the subscription for any accounts you own and want to use reliably yes. There are other cases where subscribing is useful, like when you're following an account, or looking at an account. In others it's not very useful, like when you just need an account name for an id, but due to how Dan L wrote the ChainStore it defaults to get_full_accounts no matter what if you use it to get an account object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.