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

multi: update blockchain and mempool error types. #2278

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Jul 20, 2020

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.

@davecgh davecgh added this to the 1.7.0 milestone Jul 20, 2020
Copy link
Member

@davecgh davecgh left a 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.

mempool/error.go Outdated Show resolved Hide resolved
blockchain/chain_test.go Outdated Show resolved Hide resolved
blockchain/chain_test.go Outdated Show resolved Hide resolved
blockchain/common_test.go Outdated Show resolved Hide resolved
blockchain/common_test.go Outdated Show resolved Hide resolved
blockchain/fullblocktests/generate.go Outdated Show resolved Hide resolved
blockchain/fullblocktests/generate.go Outdated Show resolved Hide resolved
blockchain/fullblocks_test.go Outdated Show resolved Hide resolved
blockmanager.go Outdated Show resolved Hide resolved
blockchain/error.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the update_blockchain_error_types branch from 619cff7 to 097911f Compare October 14, 2020 19:40
@dnldd dnldd changed the title multi: update blockchain error types. multi: update blockchain and mempool error types. Oct 14, 2020
@dnldd dnldd force-pushed the update_blockchain_error_types branch from 097911f to 44ae570 Compare October 14, 2020 19:51
Copy link
Member

@davecgh davecgh left a 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.

internal/mempool/policy_test.go Outdated Show resolved Hide resolved
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 {
Copy link
Member

@davecgh davecgh Oct 14, 2020

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.

blockmanager.go Outdated Show resolved Hide resolved
internal/mempool/error_test.go Outdated Show resolved Hide resolved
internal/mempool/error_test.go Outdated Show resolved Hide resolved
internal/mempool/error_test.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the update_blockchain_error_types branch from 44ae570 to 4caf916 Compare October 14, 2020 22:00
blockchain/error_test.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error_test.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
internal/mempool/error.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the update_blockchain_error_types branch 5 times, most recently from 10ed955 to 77245c1 Compare October 15, 2020 01:32
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.
@dnldd dnldd force-pushed the update_blockchain_error_types branch from 77245c1 to b5b371b Compare October 15, 2020 01:37
@davecgh davecgh merged commit 00933dc into decred:master Oct 15, 2020
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.

2 participants