bitcoinxt / bitcoinxt

Bitcoin XT. Most recent release is K.
MIT License
422 stars 147 forks source link

Convert signature hash type to enum class #499

Closed dagurval closed 6 years ago

dagurval commented 6 years ago

Gives signature hash types a typename, thus adding type safety.

This is a less intrusive alternative to the SigHashType enum wrapper class introduced by ABC. Also this is an alternative to BU's approach to keep both the enum and introduce ABC's class, as done in their DSV PR https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/1390

This change makes it easier to port signature encoding tests and DSV.

dgenr8 commented 6 years ago

@dagurval Could you state something about how this refactor avoids a problem similar to the one that affected ABC?

dagurval commented 6 years ago

This change is less bug prone compared to ABC's approach as it does not change the interface.

This line:

flags = SIGHASH_ALL | SIGHASH_FORKID;

is the same with this approach:

flags = SigHashType::ALL | SigHashType::FORKID;

while with ABC it's:

flags = SigHashType().withForkId();

Another example:

bool fHasSingle = (nHashType & ~(SIGHASH_ANYONECANPAY | SIGHASH_FORKID)) == SIGHASH_SINGLE

this approach:

bool fHasSingle = (nHashType & ~(SigHashType::ANYONECANPAY | SigHashType::FORKID)) == SigHashType::SINGLE

ABC is completely different:

bool fHasSingle = nHashType.withAnyoneCanPay(false).withForkId(false).getRawSigHashType() == SIGHASH_SINGLE;

Errors in updating the use of sighash flags are harder to make and easier to spot.