-
Notifications
You must be signed in to change notification settings - Fork 74
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
multi: improve handling of low fee tickets #219
Conversation
Is this the last of the bits needed to remove the remaining stake pool code out of dcrwallet? |
} | ||
|
||
// MsgTxFromHex returns a wire.MsgTx struct built from the transaction hex string | ||
func MsgTxFromHex(txhex string) *wire.MsgTx { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to return or log the error too.
@jrick This lays the foundation for getting there. I think what's left on the stake pool side is:
|
) | ||
|
||
// Public API version constants | ||
const ( | ||
semverString = "3.0.0" | ||
semverMajor = 3 | ||
grpcTimeout = time.Millisecond * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that silly short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's just copying or overwriting a map which is ~2ms on my machine. I think it could be re-written to not need a timeout.
|
||
for { | ||
select { | ||
case addedLowFeeTickets := <-s.grpcGetAddedLowFeeTicketsResultsChan: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the request should be creating this channel instead of reusing a channel in the service implementation. As it is, I don't believe this will work when multiple clients are hitting the same requests at the same time, as results received will be mixed.
goto done | ||
} | ||
|
||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't need to be a loop
|
||
done: | ||
defer func() { | ||
log.Infof("GetAddedLowFeeTickets processed %v tickets in %v waitTime %v", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is common in this codebase but i think intercepting the requests (similar to how i do it in wallet) is a much cleaner and simpler way to keep track of this info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capture the current time before and after running the handler and log the delta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using an interceptor would also be a good place to add the deadline to the context
startTime := time.Now() | ||
tickets := make([]*pb.TicketEntry, 0) | ||
|
||
// limit the time we take |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use context.WithTimeout
backend/stakepoold/rpc/api.proto
Outdated
|
||
message TicketEntry { | ||
string TicketAddress = 1; | ||
string TicketHash = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a string instead of bytes?
}() | ||
return &pb.UpdateVotingPrefsResponse{}, nil | ||
|
||
return &pb.SetUserVotingPrefsResponse{Processed: processed}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should return a non-nil error if it timed out and the request wasn't processed
This brings proper support for handling invalid/low fee tickets into the stakepool.
Basically it adds storage to the DB for saving added tickets, an admin UI to remove/add to the table, and then pushes/pulls the lists to/from stakepoold.
Not really sure about the gRPC server <-> main app ctx channel stuff. Seems to work OK but maybe there's a better way to do it.
Closes #134.
Closes #209.
Closes #218.
Also makes the fee checking fast per discussions with @chappjc and @jrick.
This also paves the way for closing some of the other open issues like #180 & #201.
Works for me™ so far but needs more testing and review.