blockchain: check errors and remove ineffectual assignments. #689
Conversation
| @@ -1044,7 +1044,6 @@ func Generate() (tests [][]TestInstance, err error) { | |||
| // \-> b38(10) | |||
| // | |||
| // OP_CHECKMULTISIG counts for 20 sigops. | |||
| tooManySigOps = repeatOpcode(txscript.OP_CHECKMULTISIG, maxBlockSigOps/20) | |||
There was a problem hiding this comment.
It looks like a merge artifact (yay merges...). Notice how this line is 100% the same as manySigOps above. Either this line can be removed like this, or it can be kept and the line below updated to append to tooManySigOps.
That said, I sort of think I'd prefer the latter since it won't break the test if the manySigOps above is changed in the future.
blockchain/subsidy.go
Outdated
| @@ -250,6 +250,9 @@ func BlockOneCoinbasePaysTokens(tx *dcrutil.Tx, | |||
| // There should only be one address. | |||
| _, addrs, _, err := | |||
| txscript.ExtractPkScriptAddrs(txout.Version, txout.PkScript, params) | |||
| if err != nil { | |||
| return err | |||
There was a problem hiding this comment.
Probably the wrong error here, see how it returns a rule error below.
There was a problem hiding this comment.
Agreed. This should return a ruleError.
|
Since this touches consensus code I want @davecgh and @marcopeereboom to give it a look before merging. |
| @@ -1044,7 +1044,6 @@ func Generate() (tests [][]TestInstance, err error) { | |||
| // \-> b38(10) | |||
| // | |||
| // OP_CHECKMULTISIG counts for 20 sigops. | |||
| tooManySigOps = repeatOpcode(txscript.OP_CHECKMULTISIG, maxBlockSigOps/20) | |||
There was a problem hiding this comment.
It looks like a merge artifact (yay merges...). Notice how this line is 100% the same as manySigOps above. Either this line can be removed like this, or it can be kept and the line below updated to append to tooManySigOps.
That said, I sort of think I'd prefer the latter since it won't break the test if the manySigOps above is changed in the future.
blockchain/chainio.go
Outdated
| @@ -920,7 +920,6 @@ func deserializeUtxoEntry(serialized []byte) (*UtxoEntry, error) { | |||
| stakeExtra := make([]byte, len(serialized[offset:])) | |||
| copy(stakeExtra, serialized[offset:]) | |||
| entry.stakeExtra = stakeExtra | |||
| offset += len(serialized[offset:]) | |||
There was a problem hiding this comment.
I'd prefer to leave this. Even though it's technically not used, if any fields are added, removing this would cause the offset to be incorrect if it weren't added back, and it's pretty likely it would be missed.
| @@ -1066,7 +1065,6 @@ func Generate() (tests [][]TestInstance, err error) { | |||
| // ... -> b39(10) | |||
| // \-> b40(11) | |||
| // | |||
| tooManySigOps = repeatOpcode(txscript.OP_CHECKMULTISIGVERIFY, maxBlockSigOps/20) | |||
There was a problem hiding this comment.
Same here as above. I think it would be less error prone to keep this line and change the line below to append to it.
blockchain/subsidy.go
Outdated
| @@ -250,6 +250,9 @@ func BlockOneCoinbasePaysTokens(tx *dcrutil.Tx, | |||
| // There should only be one address. | |||
| _, addrs, _, err := | |||
| txscript.ExtractPkScriptAddrs(txout.Version, txout.PkScript, params) | |||
| if err != nil { | |||
| return err | |||
There was a problem hiding this comment.
Agreed. This should return a ruleError.
| @@ -360,7 +360,6 @@ func TestCalcStakeVersionCorners(t *testing.T) { | |||
| bc.bestNode = currentNode | |||
|
|
|||
| } | |||
| height += runCount | |||
There was a problem hiding this comment.
Is that right? It seems like it is needed.
No description provided.