Skip to content

Remove sighash fork value (forkid)

SlowRiot requested to merge SlowRiot/bitcoin-cash-node:remove_forkid into master

Addresses #432 (closed).

The 'forkid' feature isn't used and could be removed completely. Nobody seems to currently see a use for it, and any chain that forks from Bitcoin Cash would be able to implement its own (maybe better) replay protection scheme if it wanted.

Disambiguation - "ForkID" is used to describe the signature hash type which is used after UAHF, so the majority of references in the code to ForkID will remain - it's necessary to be able to distinguish between sighashes with and without a fork ID. The value of the fork ID itself (referred in the code as a "fork value") is an arbitrary uint32_t which has always been zero. This MR removes the potential to set this to anything other than zero.

The amount of unused code that can be removed while maintaining current behaviour is quite small, and relates only to the "fork value". The only way to interact with the fork value presently is through a pair of mutation functions in sighash, and these are called nowhere outside of their own tests.

Changes

  • Remove SigHashType's fork value functions:
    • withForkValue()
    • getForkValue()
  • Remove subsections of test cases concerned with altering and checking fork value:
    • sighash_tests/sighash_test
    • sighashtype_tests/sighash_construction_test

Out of scope

There are over 200 lines of code remaining whcih refer to "fork ID", in one form or another in the source. These relate to the sighash type (all post hard fork) and are all retained. To avoid reference to an unused fork id, it may be a good idea to rename these flags in future.

Similarly, the flags SCRIPT_ENABLE_SIGHASH_FORKID and bitcoinconsensus_SCRIPT_ENABLE_SIGHASH_FORKID remain as they are used by logic which decides whether a fork ID should be present or not. Again, renaming these in future to something related to the HF could be a good idea.

Test plan

ninja check-all

Merge request reports