Skip to content

Add bip94 setting for Testnet4 #792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thecodefactory wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add bip94 setting for Testnet4 #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f1f657
Add bip94 setting for Testnet4
thecodefactory Jun 6, 2026
7b1f615
Adjusting settings to see if CI is happy
thecodefactory Jun 7, 2026
c22f4bc
Update msvc as well
thecodefactory Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions builds/cmake/install-cmake.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ if [[ -z ${secp256k1_TAG} ]]; then
fi

if [[ -z ${libbitcoin_system_OWNER} ]]; then
libbitcoin_system_OWNER="libbitcoin"
libbitcoin_system_OWNER="thecodefactory"

@evoskuil evoskuil Jun 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artifacts?

@thecodefactory thecodefactory Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 evoskuil Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi
if [[ -z ${libbitcoin_system_TAG} ]]; then
libbitcoin_system_TAG="master"
libbitcoin_system_TAG="testnet4"
fi

if [[ -z ${libbitcoin_database_OWNER} ]]; then
Expand All @@ -91,10 +91,10 @@ if [[ -z ${libbitcoin_network_TAG} ]]; then
fi

if [[ -z ${libbitcoin_node_OWNER} ]]; then
libbitcoin_node_OWNER="libbitcoin"
libbitcoin_node_OWNER="thecodefactory"
fi
if [[ -z ${libbitcoin_node_TAG} ]]; then
libbitcoin_node_TAG="master"
libbitcoin_node_TAG="testnet4"
fi

if [[ -z ${libbitcoin_server_OWNER} ]]; then
Expand Down
8 changes: 4 additions & 4 deletions builds/gnu/install-gnu.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ if [[ -z ${secp256k1_TAG} ]]; then
fi

if [[ -z ${libbitcoin_system_OWNER} ]]; then
libbitcoin_system_OWNER="libbitcoin"
libbitcoin_system_OWNER="thecodefactory"
fi
if [[ -z ${libbitcoin_system_TAG} ]]; then
libbitcoin_system_TAG="master"
libbitcoin_system_TAG="testnet4"
fi

if [[ -z ${libbitcoin_database_OWNER} ]]; then
Expand All @@ -91,10 +91,10 @@ if [[ -z ${libbitcoin_network_TAG} ]]; then
fi

if [[ -z ${libbitcoin_node_OWNER} ]]; then
libbitcoin_node_OWNER="libbitcoin"
libbitcoin_node_OWNER="thecodefactory"
fi
if [[ -z ${libbitcoin_node_TAG} ]]; then
libbitcoin_node_TAG="master"
libbitcoin_node_TAG="testnet4"
fi

if [[ -z ${libbitcoin_server_OWNER} ]]; then
Expand Down
8 changes: 4 additions & 4 deletions builds/msvc/build-msvc.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ if "!NUGET_EXE!" == "" (
)

if "!libbitcoin_system_OWNER!" == "" (
set "libbitcoin_system_OWNER=libbitcoin"
set "libbitcoin_system_OWNER=thecodefactory"
)
if "!libbitcoin_system_TAG!" == "" (
set "libbitcoin_system_TAG=master"
set "libbitcoin_system_TAG=testnet4"
)

if "!libbitcoin_database_OWNER!" == "" (
Expand All @@ -60,10 +60,10 @@ if "!libbitcoin_network_TAG!" == "" (
)

if "!libbitcoin_node_OWNER!" == "" (
set "libbitcoin_node_OWNER=libbitcoin"
set "libbitcoin_node_OWNER=thecodefactory"
)
if "!libbitcoin_node_TAG!" == "" (
set "libbitcoin_node_TAG=master"
set "libbitcoin_node_TAG=testnet4"
)

if "!libbitcoin_server_OWNER!" == "" (
Expand Down
5 changes: 5 additions & 0 deletions src/parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ options_metadata parser::load_settings() THROWS
value<bool>(&configured.bitcoin.forks.bip90),
"Assume bip34, bip65, and bip66 activation if enabled, defaults to 'true' (hard fork)."
)
(
"forks.bip94",
value<bool>(&configured.bitcoin.forks.bip94),
"Assume bip94 activation if enabled, defaults to 'true' (hard fork)."

@evoskuil evoskuil Jun 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thecodefactory Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@evoskuil evoskuil Jun 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thecodefactory Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thecodefactory Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
(
"forks.bip68",
value<bool>(&configured.bitcoin.forks.bip68),
Expand Down
1 change: 1 addition & 0 deletions src/protocols/native/protocol_native_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ bool protocol_native::handle_get_block_header_context(const code& ec,
{ "bip66", context.is_enabled(chain::flags::bip66_rule) },
{ "bip65", context.is_enabled(chain::flags::bip65_rule) },
{ "bip90", context.is_enabled(chain::flags::bip90_rule) },
{ "bip94", context.is_enabled(chain::flags::bip94_rule) },
{ "bip68", context.is_enabled(chain::flags::bip68_rule) },
{ "bip112", context.is_enabled(chain::flags::bip112_rule) },
{ "bip113", context.is_enabled(chain::flags::bip113_rule) },
Expand Down
Loading