Skip to content
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.

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

Fix "Unknown new rules activated" Error & Uninitialized Variable nMinerConfirmationWindow, Revert Changes #76

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

JaredTate
Copy link

@JaredTate JaredTate commented Oct 19, 2021

After digging in deep into the "Unknown new rules activated" issue we have actually revealed multiple unknown issues we can now correct. So I really appreciate Barry's help as well as others feedback on this. We definitely have some great collaboration here. This PR is a continuation of Barrys PR: #75

The issue with adding:

            // dont trigger 'unknown new rules' warning if the bit falls within
            // the block algo version range (enum in primitives/block.h)
            if (bit <= BLOCK_VERSION_ALGO) continue;   

Is that then causes an issue for potential future algo swaps as there are reserved bits below BLOCK_VERSION_ALGO.

enum {
    // primary version
    BLOCK_VERSION_DEFAULT        = 2, 

    // algo
    BLOCK_VERSION_ALGO           = (15 << 8),
    BLOCK_VERSION_SCRYPT         = (0 << 8),
    BLOCK_VERSION_SHA256D        = (2 << 8),
    BLOCK_VERSION_GROESTL        = (4 << 8),
    BLOCK_VERSION_SKEIN          = (6 << 8),
    BLOCK_VERSION_QUBIT          = (8 << 8),
    //BLOCK_VERSION_EQUIHASH       = (10 << 8),
    //BLOCK_VERSION_ETHASH         = (12 << 8),
    BLOCK_VERSION_ODO            = (14 << 8),
};

If anything it should be Odo specific:

            // dont trigger 'unknown new rules' warning if the bit falls within
            // the block algo version range (enum in primitives/block.h)
            if (bit == BLOCK_VERSION_ODO) continue;   

However the easier fix is much more nuanced. It appears BTC devs ran into similar issue and the problem is a un initialized variable "nMinerConfirmationWindow" and we need to hard code the consensus.MinBIP9WarningHeight. Bitcoin devs corrected and fixed a similar issue here: bitcoin/bitcoin#17449

So basically this needs changed

consensus.MinBIP9WarningHeight = consensus.OdoHeight + consensus.nMinerConfirmationWindow;   

To

consensus.MinBIP9WarningHeight = 9152640; // Odo height + miner confirmation window,;   

Barry identified a message in his PR about VERSIONBITS_NUM_BITS_TO_SKIP which shows we never fully reverted test changes made in 2017.

VERSIONBITS_NUM_BITS_TO_SKIP was actually reverted the very next day they were proposed, however the one thing never reverted was static const int32_t VERSIONBITS_NUM_BITS = 16.

That should be 29. So 29 should be the max number of potential version bits. Also, Barrys commits in here fully bury Odocrypt deployment as they should be.

Thanks to everyone for your help looking into this. This was the last major issue I have been able to identify with 8.22. My recommendation is we get this merged into the develop branch and test mining, and taproot activation. Once there we are done with BTC v22 merge and can move on to other v8.22 enhancements.

barrystyle and others added 3 commits October 8, 2021 02:38
This is not required if the deployment has been buried.
Additionally it causes a segfault if digibyte was launched with a new data directory.
Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

ACK

Great work on this @JaredTate and @barrystyle . I tested and can confirm there are no further "Unknown new rules activated" messages.

@ycagel
Copy link
Member

ycagel commented Oct 26, 2021

cACK

@gto90 gto90 requested a review from ycagel November 1, 2021 15:42
@ycagel ycagel merged commit 725ea92 into DigiByte-Core:feature/8.22.0 Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants