chore: Do not convert int to uint32 in btcstaking lib #135
Conversation
vitsalis
requested review from
KonradStaniec and
Lazar955
and removed request for
a team
vitsalis
force-pushed
the
vitsalis/type-conversions-btc-library
branch
from
ed1e4bb
to
ab85b5a
Compare
| @@ -45,7 +46,11 @@ func newLightChainCtxFromParams(params *chaincfg.Params) *lightChainCtx { | |||
| targetTimePerBlock := int64(params.TargetTimePerBlock / time.Second) | |||
| adjustmentFactor := params.RetargetAdjustmentFactor | |||
|
|
|||
| blocksPerRetarget := int32(targetTimespan / targetTimePerBlock) | |||
There was a problem hiding this comment.
tbh this is what btcd does with their types regarding those values, and I would leave it like that. I assume they try to match whatever bitcoind is doing. (ref:https://github.com/btcsuite/btcd/blob/master/blockchain/chain.go#L2186)
And in this case it is better to be consistent with btcd and bitcoind, than be consistent with gosec.
There was a problem hiding this comment.
Aren't we doing the same thing though? No problem with keeping it as it is though, did it just for gosec, but we can add a nosec tag if want to. Reverted.
There was a problem hiding this comment.
Aren't we doing the same thing though?
Most probably we are doing the same thing, but the probably part would require some investigation to be 100% sure, given that this values are relevant to consensus of BTC headers.
On the other hand, being compatible with btcd relaxes this a bit (I think there are 60 btcd nodes in bitcoin network rn, so if they are conensus compliant we are off the hook) 😅
vitsalis
force-pushed
the
vitsalis/type-conversions-btc-library
branch
from
63dfbb3
to
f619a3e
Compare
This PR handles the conversion of ints to uint32s in the btcstaking library. The conversions that were modified were not unsafe as they were the results of the `len()` call, but they still made static analyzers complain. Instead, we opted to convert uint32s to ints. However, this makes the assumption that the program will be executed in 64-bit architectures in order to be safe, which afaik is already a requirement. Further, the PR includes some other small conversion fixes that have been found in the codebase -- mostly low hanging fruit.
vitsalis
force-pushed
the
vitsalis/type-conversions-btc-library
branch
from
f619a3e
to
39bd50e
Compare
This PR handles the conversion of ints to uint32s in the btcstaking library. The conversions that were modified were not unsafe as they were the results of the
len()call,but they still made static analyzers complain.
Instead, we opted to convert uint32s to ints. However, this makes the assumption that the program will be executed in 64-bit architectures in order to be safe, which afaik is already a requirement.