Add bip94 setting for Testnet4

[thecodefactory]

Depends on libbitcoin/libbitcoin-system#1866 and libbitcoin/libbitcoin-node#1034

[thecodefactory]

I am able to sync testnet4 with a config like this:

[peer]
identifier = 675223068

[bitcoin]
genesis_block = 0100000000000000000000000000000000000000000000000000000000000000000000004e7b2b9128fe0291db0693af2ae418b767e657cd407e80cb1434221eaea7a07a046f\
3566ffff001dbb0c78170101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff5504ffff001d01044c4c30332f4d61792f323032342030303030\
303030303030303030303030303030303165626435386332343439373062336161396437383362623030313031316662653865613865393865303065ffffffff0100f2052a010000002321000000\
000000000000000000000000000000000000000000000000000000000000ac00000000
milestone = 00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043:0
#milestone = 0000000002368b1e4ee27e2e85676ae6f9f9e69579b29093e9a82c170bf7cf8a:123613 
checkpoint = 00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043:0
#checkpoint = 0000000002368b1e4ee27e2e85676ae6f9f9e69579b29093e9a82c170bf7cf8a:123613 
proof_of_work_limit = 486604799
minimum_work = 000000000000000000000000000000000000000000000b6a51f415a67c0da307
bip16_activation_time = 1329264000
bip34_activation_threshold = 51
bip34_enforcement_threshold = 75
bip34_activation_sample = 100
bip30_reactivate_height = 486000000
bip30_deactivate_checkpoint = 0000000012982b6d5f621229286b880e909984df669c2afabb102ce311b13f28:1
bip9_bit0_active_checkpoint = 0000000012982b6d5f621229286b880e909984df669c2afabb102ce311b13f28:1
bip9_bit1_active_checkpoint = 0000000012982b6d5f621229286b880e909984df669c2afabb102ce311b13f28:1
bip9_bit2_active_checkpoint = 0000000012982b6d5f621229286b880e909984df669c2afabb102ce311b13f28:1

[forks]
bip94 = true
retarget = true
difficult = false

[thecodefactory]

@pmienk What's the proper way to have PRs like this with dependent branches complete?

[evoskuil]

retarget = true

This is a default (mainnet), no need to include.

[evoskuil]

artifacts?

[thecodefactory]

No, I was trying to see if it would pull my branches to pass CI, but I'll remove that stuff because it didn't work anyway

[evoskuil]

If you merge them all in your own repos you can get a result spanning all of them. This was a change that @pmienk make a few months ago. As for the origin repo, there's no better answer for that - but if you pull them all it should generally be after you've verified them all in your repo. Then when they are all good to go we can merge them all quickly in origin.

[evoskuil]

This should be moved to the top section with these:

    /* [forks] */
    (
        "forks.difficult",
        value<bool>(&configured.bitcoin.forks.difficult),
        "Require difficult blocks, defaults to 'true' (use false for testnet)."
    )
    (
        "forks.retarget",
        value<bool>(&configured.bitcoin.forks.retarget),
        "Retarget difficulty, defaults to 'true' (use false for regtest)."
    )

or to the bottom with these:

    (
        "forks.time_warp_patch",
        value<bool>(&configured.bitcoin.forks.time_warp_patch),
        "Fix time warp bug, defaults to 'false' (hard fork)."
    )
    (
        "forks.retarget_overflow_patch",
        value<bool>(&configured.bitcoin.forks.retarget_overflow_patch),
        "Fix target overflow for very low difficulty, defaults to 'false' (hard fork)."
    )
    (
        "forks.scrypt_proof_of_work",
        value<bool>(&configured.bitcoin.forks.scrypt_proof_of_work),
        "Use scrypt hashing for proof of work, defaults to 'false' (hard fork)."
    )

Assume bip94 activation if enabled, defaults to 'true' (hard fork).
Must default to false, the "assume activation" text isn't applicable here, should be more similar to the litecoin comments.

BIP fork settings are named so that they can default to true (as are retarget and difficult). If this one does as well (as described in its text) it would need to be named not_bip94). The reason for this is that we want the flags representation (e.g. in the store and in the console log for example) to reflect all zeros until a fork activates. It might be better to not actually call this bip94.

Also in bip94:

1. 20-minute Exception Rule
   This rule was implemented in Testnet 3[2] and is preserved in Testnet 4.

So as you showed in the config, this part of bip94 is implemented in a distinct fork. And this config is therefore activating a pair of additional forks ("Block Storm Fix", "Time Warp Attack Prevention"). Which means by default, there is a "block storm" behavior and a "time warp" behavior, that are enabled, and this fork disables them.

Note that we have these three settings for litecoin:

    /// Litecoin rules.
    /// -----------------------------------------------------------------------

    /// Fix Satoshi's time warp bug (hard fork, security).
    time_warp_patch = bit_right<uint32_t>(17),

    /// Fix target overflow for very low difficulty (hard fork, security).
    retarget_overflow_patch = bit_right<uint32_t>(18),

    /// Use scrypt hashing for proof of work (hard fork, feature).
    scrypt_proof_of_work = bit_right<uint32_t>(19),

    forks.time_warp_patch = false;         // litecoin
    forks.retarget_overflow_patch = false; // litecoin
    forks.scrypt_proof_of_work = false;    // litecoin

[And we are using up our 32 flag space (impacts database schema/size).]

bip94 describes a second "time warp patch" (not sure why this different one was chosen, ltc has had this forever).

This rule is based on The Great Consensus Cleanup proposal.

This implies that this could become non-testnet rule. I would not implement the "cleanup" rules as one rule either, as they are entirely independent rules. So this "time warp patch" could just eventually be re-labelled/activated as one of those.

JR has proposed independent BIPs for the three "cleanup" forks. Might be worth taking the name from that one. In any case we need to deconflict with time_warp_patch that we already have.

bip94 also defines a "block storm patch" to the problems cause by the !difficult testnet3 fork. Since it describes these as two distinct rules they probably should be. So for this one I would call the flag:

/// Address block storms caused by !difficult rule (hard fork, testnet4).
block_storm_patch = bit_right<uint32_t>(...)

[thecodefactory]

This was a mistake: "Assume bip94 activation if enabled, defaults to 'true' (hard fork)." -- It should in fact default to false

[evoskuil]

To resolve the naming conflicts above I recommend the approach of prefixing altcoin-only forks with the a chain identifier, eg:

    forks.ltc_time_warp_patch = false;
    forks.ltc_retarget_overflow_patch = false;
    forks.ltc_scrypt_proof_of_work = false;

And grouping them at the bottom (high end) of flags (so they appear together as 1) in the bit matrix.

Which frees up this distinct time_warp_patch name, which can be combined with block_storm_patch. I would implement these as distinct forks for btc (unprefixed), and set them in a group just after ltc (for now, this preserves compat with existing stores), for both flags and forks enums.

So a distinct set of PRs to add ltc_ and then rebase over that.

[thecodefactory]

I'm not opposed to splitting things up more with additional settings/flags, but there are 2 reasons I think we shouldn't: 1) bip94 is either implemented/enforced, or it's not (there's no independent path for the changes so far, at least that's specified in that bip), and 2) as you noted, we're running out of flag bits. I'm ok with not_bip94 based on the fact that we want the setting to be true by default. I'm also not that strongly tied to the setting names, so if you have a preference for both or just a single one, I'll go with that.

[thecodefactory]

Just saw your previous comment now (sorry, didn't reload the page I guess before posting my last response). Sounds good though