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

Should Expired by included in total Missed count? or Seperate? #371

Closed
alexlyp opened this issue Oct 24, 2016 · 14 comments
Closed

Should Expired by included in total Missed count? or Seperate? #371

alexlyp opened this issue Oct 24, 2016 · 14 comments

Comments

@alexlyp
Copy link
Member

alexlyp commented Oct 24, 2016

We've been trying to figure out if Missed count should no longer include the new Expired count.

Thoughts?

My main worry is if someone has an existing infrastructure that uses missed count elsewhere then it will all of a sudden get reduced. Could cause issues

@raedah
Copy link
Contributor

raedah commented Oct 25, 2016

My thought was that expired would not be part of missed. Missed is helpful in tracking what could have been avoided, and expired is completely out of the users control. I cant think of any obvious infrastructure that uses missed count, but even if they did, its still beta software! ;)

@alexlyp
Copy link
Member Author

alexlyp commented Oct 25, 2016

Yah good points

@ghost
Copy link

ghost commented Nov 1, 2016

Do I understand right, that currently:
Missed = Missed + Expired
Expired = Expired

Of course, it is a little bit confusing, but I like this approach. I've updated only 1 stakepool wallet to master branch and it's working as expected. I like that there is no mismatch in the getstakeinfo output of the old version and the new one, so we can use both versions with no synchronization issues on the dcrstakepool part.

@alexlyp
Copy link
Member Author

alexlyp commented Nov 1, 2016

@dyrk yah that was roughly my take. I'd like things to stay consistent over updates which could throw off analytics that have been accumulating since 0

@raedah
Copy link
Contributor

raedah commented Nov 1, 2016

I do not like having to explain technical debt/legacy code to people coming onto the project with saying oh thats confusing because it was left that way to stay compatible with the way it was being done before version 0.5. Maybe the solution to this is if getstakeinfo took a blocknumber as a parameter, a user could rebuild historical data whenever they needed it.

@ghost
Copy link

ghost commented Nov 1, 2016

I do agree that we shouldn't support that legacy code, so is better to keep missed and expired ticket separately. But please do not add blockheight parameter to the getstakeinfo, it is already slow as hell :) I don't know if it's possible to optimize this call and reduce amount of RPC requests, but in my pool sometimes it takes up to 10 seconds already to calculate all stakeinfo stats.

@chappjc
Copy link
Member

chappjc commented Nov 1, 2016

Agree on that last comment. Although not necessarily about getstakeinfo, blockheight isn't likely a big computation. :)

@raedah
Copy link
Contributor

raedah commented Nov 1, 2016

@dyrk Sorry I am not understanding you position. "please do not add blockheight parameter to the getstakeinfo, it is already slow as hell." I do not see any reason to believe that having a parameter would make it slower.

@ghost
Copy link

ghost commented Nov 1, 2016

@raedah I was saying that is better to optimize it first and then add new features to the getstakeinfo call.

@chappjc
Copy link
Member

chappjc commented Nov 2, 2016

Just to reiterate my opinion expressed in #360 , I'd prefer to see expired not included in missed. However, I understand the reasons to avoid changing the semantics of a value. But in this case, I suspect it's not really going to break anything if the missed counts drop. The case to ease pool updates is not too persuasive to me, as it's a short-lived concern.

@David00
Copy link

David00 commented Nov 27, 2016

Now that we have the expired count in getstakeinfo, I would like to see the value removed from the missed count. To me, missed should mean something was wrong with the network or my voting node, causing it to miss votes. I think raedah said it very well above: expired tickets are completely out of the users control whereas missing a vote means there is some action the user could have been taken to avoid missing that vote.

@davecgh
Copy link
Member

davecgh commented Nov 30, 2016

I personally don't think expired should be part of missed either, though I certainly understand the desire to avoid any potential issues with the semantic change.

That said, I think as long as the semantic change is properly documented and loudly announced ahead of time in order to give anyone who happens to rely on it time to make the necessary changes before it's released, and announced again at release time, the change should be made.

Along these same lines, one of the key benefits of Decred is the ability to use the PoS voting system to enable consensus approved hard forks. Hard forks will necessarily require ecosystem updates, so I don't think a logical and useful change such as this one is out of the ball park.

Consequently, in general, I don't think an expectation of never changing anything and amassing technical debt makes as much sense in Decred. That is not to say things should be changed for the sake of change without sound reasoning however as that can get annoying to all API consumers incredibly quickly.

@jrick
Copy link
Member

jrick commented May 1, 2017

I'd like to see this change happen instead of it lingering forever. We will try to do better about informing users of breaking changes in future releases.

@hearinleaf
Copy link

Missed < Expired

beansgum pushed a commit to beansgum/dcrwallet that referenced this issue Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants