btclightclient: Cleanup codebase and add descriptive comments #50
Conversation
| prevHeader, err := k.HeadersState(sdkCtx).GetTip() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
How can this fail? We start from genesis with a trusted header, and we always extend with valid blocks. There shouldn't be a semantic failure coming from it, right?
There was a problem hiding this comment.
It shouldn't fail. Actually, the GetTip() function only fails in the case that the tip is malformed. With that in mind, I will remove the returned error from the GetTip() method and add a panic.
x/btclightclient/keeper/state.go
Outdated
| // Get necessary keys according | ||
| headersKey, err := types.HeadersObjectKey(height, &headerHash) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| heightKey, err := types.HeadersObjectHeightKey(&headerHash) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
How can these fail? We have a valid wire.BlockHeader so headerHash is legit. Why couldn't we generate keys from it.
There was a problem hiding this comment.
These fail if the headerHash is not properly serialized into bytes. The chainhash.NewHash method returns an error and we handle the error by returning it. Do you think this should be a panicking occasion?
There was a problem hiding this comment.
I mean, we are using these 3rd party types because we assume they do the right thing. I'm not sure what chainhash.NewHash is called with this time, but if it's the hash calculated by wire.BlockHeader, wouldn't it be a WTF moment if it complained about it? If it was some random bytes from a gRPC request, then it can rightfully fail, but we should catch those early, so that the rest of the code can stop second guessing every step along the way.
x/btclightclient/keeper/state.go
Outdated
| headersKey, err := types.HeadersObjectKey(height, hash) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
It looks like hash is a domain type already, presumably validated. Why can't we turn it into a key with confidence? What can we do about such an error?
x/btclightclient/keeper/state.go
Outdated
| // Get the BTCHeaderBytes object | ||
| headerBytes, err := bbl.NewBTCHeaderBytesFromBytes(rawBytes) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This is a record from our database, why couldn't we turn it back into headers?
x/btclightclient/keeper/state.go
Outdated
| // Keyed by hash | ||
| hashKey, err := types.HeadersObjectHeightKey(hash) | ||
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
Same question of how this can fail.
There was a problem hiding this comment.
It is a good question of what should return an error.
On one hand, there's the philosophy of pushing out error handling to the caller, let them deal with it.
On the other, Go is an abysmal language when it comes to errors, it adds a lot of noise. And arguably in the blockchain context, if there is nothing we can do about the error, we don't necessarily want to reach consensus over how the application doesn't work, that is, if we return an error as a result of a transaction because we can't deserialise the tip from the database - that has nothing to do with the error, and hopefully doesn't affect all nodes. If it does, then we'll falsely make the user pay for what is not their fault.
So, perhaps it's better to be more restrained and only use errors where the problem was user input.
I'd defer to @SebastianElvis and @gitferry on how to go about this PR.
|
Thanks @aakoshh for the comments. I agree that some of those shouldn't be errors and those errors should not happen if the application works properly. However, many methods that are used throughout the codebase return errors (mainly those that are implemented by the The main question is: Should we leave those errors unhandled? In some occassions (mostly related to already stored data that should have been validated first), those errors should lead to panics because they definitely shouldn't happen. |
types/btc_header_hash_bytes.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return headerHashBytes, nil |
There was a problem hiding this comment.
I think this could panic. We have a domain type in chainhash.Hash. If that doesn't match what we think the correct length is, something is very wrong.
|
I know it's cumbersome but maybe a |
|
@aakoshh Just pushed a major refactoring in which I follow a "fortress" approach. Basically, in order to become a This allowed to remove the bulk of errors and also improve type safety. |
Just some minor improvements based on previous changes and a set of (maybe overly descriptive) comments.