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

blockchain: Finish TODO by improving error message. #540

Merged
merged 1 commit into from
Nov 9, 2015

Conversation

jrick
Copy link
Member

@jrick jrick commented Nov 6, 2015

Removes a TODO by improving the error message.

In the presence of overflow, the actual output sum is still not negative and should not be reported as so.

I also realize that for bitcoin the overflow would have never occurred to begin with, but in other cryptocurrencies 2*MaxBaseUnits (whatever they call them) may overflow an int64 and the total in the error message would be negative.

Review on Reviewable

@davecgh
Copy link
Member

davecgh commented Nov 6, 2015

After reviewing this and discussing it in IRC, I believe we can simply remove the TODO.

The reason is that while it's true it's only adding positive values and therefore can't be negative under normal circumstances, because it's an int64, if it overflows, it will become negative and thus the negative check will properly detect it and give an error message. However, we might want to change the error message to indicate overflow since that is what it means.

For example, consider the code with some smaller values:

package main

import "fmt"

const MaxSatoshi = 125

func main() {
    var totalSatoshi int8
    // The total here would sum to 140, but since it overflows an int8, it'll trigger the error.
    for i := 0; i < 7; i++ {
        satoshi := int8(20)
        // Range checks that are already there
        totalSatoshi += satoshi
        if totalSatoshi < 0 {
            fmt.Printf("total value of all transaction outputs has negative value of %v\n", totalSatoshi)
            return
        }
        if totalSatoshi > MaxSatoshi {
            fmt.Printf("total value of all transaction outputs is %v which is higher than max allowed value of %v", totalSatoshi, MaxSatoshi)
            return
        }
    }
}

Output:

total value of all transaction outputs has negative value of -116


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@jrick jrick force-pushed the unnecessary_check branch 3 times, most recently from 8614b8f to 8f463bc Compare November 7, 2015 00:54
@davecgh
Copy link
Member

davecgh commented Nov 8, 2015

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@davecgh davecgh changed the title blockchain: Remove check of impossible condition. blockchain: Finish TODO by improving error message. Nov 8, 2015
@davecgh
Copy link
Member

davecgh commented Nov 8, 2015

OK, but needs a rebase.

In the presence of overflow, the actual output sum is still not
negative and should not be reported as so.
@conformal-deploy conformal-deploy merged commit 23bcb36 into btcsuite:master Nov 9, 2015
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.

3 participants