-
Notifications
You must be signed in to change notification settings - Fork 155
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
rpc: Add RevokeTicket grpc for SPV mode #2061
Conversation
wallet/tickets.go
Outdated
// know it is a revocation. | ||
watchOutPoints, err = w.processTransactionRecord(ctx, dbtx, rec, nil, nil) | ||
if err != nil { | ||
return errors.E(op, err) |
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.
return err
to keep similar to the other instances.
wallet/tickets.go
Outdated
} | ||
revocations := make([]*wire.MsgTx, 0, 1) | ||
revocations = append(revocations, revocation) | ||
return p.PublishTransactions(ctx, revocations...) |
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.
Is publishing being done inside the db tx intentional? If this errors it's going to roll back the prior processTransactionRecord
.
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.
Hrm no not necessarily. I see RevokeTickets has the PublishTransaction within the Update and RevokeExpiredTickets doesn't. So wasn't really sure which is the preferred way to go.
func (s *walletServer) RevokeTicket(ctx context.Context, req *pb.RevokeTicketRequest) (*pb.RevokeTicketResponse, error) { | ||
var ticketHash *chainhash.Hash | ||
var err error | ||
if len(req.TicketHash) != 0 { |
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.
You also need to return early from this call in an else
block, otherwise you'll end up passing a nil ticketHash
to RevokeTicket
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.
Tested fine when running from Decrediton's #3506
wallet/tickets.go
Outdated
revocations := make([]*wire.MsgTx, 0, 1) | ||
revocations = append(revocations, revocation) | ||
err = p.PublishTransactions(ctx, revocations...) |
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.
revocation
is already a *wire.MsgTx
and Peer.PublishTransactions
is variadic, so this can be simplified to err = p.PublishTransactions(ctx, revocation)
without the need to create the slice.
internal/rpc/rpcserver/server.go
Outdated
@@ -1822,6 +1822,8 @@ func (s *walletServer) RevokeTicket(ctx context.Context, req *pb.RevokeTicketReq | |||
if err != nil { | |||
return nil, status.Errorf(codes.InvalidArgument, "%v", err) | |||
} | |||
} else { | |||
return nil, status.Errorf(codes.InvalidArgument, "%v", err) |
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.
err
is always going to be nil
in this case, so it's maybe better to return a literal TicketHash is a required argument
or something to that effect.
099a82d
to
4257a96
Compare
internal/rpc/rpcserver/server.go
Outdated
return nil, status.Errorf(codes.InvalidArgument, "tickethash is required to revoke ticket") | ||
} | ||
|
||
if len(req.Passphrase) > 0 { |
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.
we no longer submit the passphrase to unlock the wallet for each separate request, because it does not work properly after we introduced per-account encryption. instead, this call should require that the wallet (or account) is already unlocked, and error otherwise.
wallet/tickets.go
Outdated
return err | ||
} | ||
if !owned || !havePrivKey { | ||
return 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.
trying to revoke a ticket that we don't own, or don't have the key available for, should result in an error.
9c5f841
to
fcb8f83
Compare
This will now allows users to Revoke missed tickets while wallets are in SPV mode.
The user may have to abandon the revoke transaction if it wasn't actually able to be revoked
(and is therefore invalid).