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

Ensure low fee tickets are detected upon maturation. #524

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

jholdstock
Copy link
Member

The low fee tickets feature has been broken since inception.
There are multiple places where tickets are validated and
they had inconsistent error handling.

This PR also includes some logging and commenting I added
whilst investigating the issue.

Closes #521

The low fee tickets feature has been broken since inception.

This PR also includes some logging and commenting I added
whilst investigating the issue.
@jholdstock
Copy link
Member Author

Draft because this worked well for me in dev, and I was able to test on testnet briefly, but then blocks stopped coming in. Will mark for review once properly tested on testnet

@jholdstock jholdstock marked this pull request as ready for review September 13, 2019 10:52
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Still waiting on blocks to see if it works.

backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved
@@ -521,21 +526,22 @@ func (ctx *AppContext) processNewTickets(nt NewTicketsForBlock) {

msgTx, err := MsgTxFromHex(n.hex)
if err != nil {
log.Warnf("MsgTxFromHex failed for %v: %v", n.hex, err)
log.Warnf("processNewTickets: MsgTxFromHex failed for %v: %v", n.hex, err)
Copy link
Member

Choose a reason for hiding this comment

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

If we end up adding an error package like suggested in #518 , we might also start using the convention in dcrwallet where the operation is declared at the beginning of a function:
const op = "processNewTickets"
Then when we make an error
err := fmt.Errorf("%v: everybody died", op)
Just a suggestion tho, we can always decide later when/if that issue gets worked on.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can go through and do this later in #518. I might actually make a start on that this afternoon

@JoeGruffins
Copy link
Member

JoeGruffins commented Sep 13, 2019

It works!

@jholdstock BUT, after adding the tickets, they still show as invalid for the user. Do you see the same?

@jholdstock
Copy link
Member Author

Thanks all for the reviews. I have implemented all suggestions apart from the error change from Joe, I will implement that in #518

after adding the tickets, they still show as invalid for the user. Do you see the same?

Yeah I see this as well. This was how the original implementation worked. We could possibly change this later, in another PR, but I don't think it is worth investing much more time into this feature. Having spoken with VSP admins, it seems like the feature is not really used since VSP staking was added to decrediton.

@dajohi dajohi merged commit caa510a into decred:master Sep 16, 2019
girino added a commit to girino/dcrstakepool that referenced this pull request Sep 19, 2019
* commit '0cec13529c159059eff39dc6eb5b69cde5c1bd2d':
  Add 1.2.0 release note. (decred#509)
  multi: cleanup (decred#527)
  Ensure low fee tickets are detected upon maturation. (decred#524)
  Add RPC automatic reconnections (decred#510)
  Prevent unnecessary wallet rescans. (decred#519)
  enablevoting=0 in dcrwallet conf (decred#520)
  stakepoold: Stop if wallet voting is enabled (decred#523)
jyap808 pushed a commit to ubiq/dcrstakepool that referenced this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low fee tickets isnt working
4 participants