-
Notifications
You must be signed in to change notification settings - Fork 298
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: update blockchain and mempool error types. #2278
Conversation
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.
Thanks for the PR. It looks pretty good overall. There are a couple of incorrect cases I identified inline as well as many opportunities to simplify the code with by making full use of the new infrastructure also identified inline.
619cff7
to
097911f
Compare
097911f
to
44ae570
Compare
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.
Round 1 of latest updates.
blockmanager.go
Outdated
@@ -633,7 +633,7 @@ func errToWireRejectCode(err error) (wire.RejectCode, string) { | |||
switch { | |||
case errors.As(err, &berr): | |||
// Convert the chain error to a reject code. | |||
switch berr.ErrorCode { | |||
switch berr.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.
This is still not really correct. Now that both blockchain
and mempool
use the new error infrastructure, I'd suggest simplifying all this to just be.
func errToWireRejectCode(err error) (wire.RejectCode, string) {
// The default reason to reject a transaction/block is due to it being
// invalid somehow.
code := wire.RejectInvalid
var reason string
// Convert recognized errors to a reject code.
switch {
// Rejected due to duplicate.
case errors.Is(err, blockchain.ErrDuplicateBlock):
code = wire.RejectDuplicate
reason = err.Error()
// Rejected due to obsolete version.
case errors.Is(err, blockchain.ErrBlockVersionTooOld):
code = wire.RejectObsolete
reason = err.Error()
// Rejected due to checkpoint.
case errors.Is .... the rest
...
// Error codes which map to a duplicate transaction .....
case errors.Is(err, mempool.ErrMempoolDoubleSpend),
errors.Is(err, mempool.ErrAlreadyVoted),
.... the rest:
code = wire.RejectCheckpoint
reason = err.Error()
default:
reason = fmt.Sprintf("rejected: %v", err)
}
return code, reason
}
In other words, there is no longer a need to manually unwrap and check things. Just check for the recognized things directly and let errors.Is
handle the unwrapping.
44ae570
to
4caf916
Compare
10ed955
to
77245c1
Compare
This updates the blockchain and mempool error types to leverage go 1.13 errors.Is/As functionality as well as confirm to the error infrastructure best practices.
77245c1
to
b5b371b
Compare
This updates the blockchain and mempool error types to leverage go 1.13 errors.Is/As functionality as well as confirm to
the error infrastructure best practices.