Skip to content

Conversation

@AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Apr 23, 2025

Description

Problem:

Consider a hardfork (Echidna in our case) that is:

  1. Explicitly omitted from the node configuration;
  2. Given the fact that the set of previous hardforks are explicitly enabled in the node configuration.

The problem is that this hardfork is treated as "enabled from genesis" by IsInitializeBlock function. This problem is discovered when running N3 mainnet/testnet node with the default configuration on the current master. This behaviour leads to unexpected Echidna-related changes being included into the node state starting from genesis block. Here are some data:

Hardforks configuration that is used in config.json file (given this config, Echidna should be treated as disabled, but in fact it is treated as enabled by IsInitializeBlock method):

    "Hardforks": {
      "HF_Aspidochelone": 210000,
      "HF_Basilisk": 2680000,
      "HF_Cockatrice": 3967000,
      "HF_Domovoi": 4144000
    },

Genesis block state difference between the current master and 3.7 version:

Processing directory BlockStorage_0
file BlockStorage_0/dump-block-0.json: block 0, changes length mismatch: 34 vs 41

In particular, there are 7 extra storage items stored in genesis. All these changes relate to new functionality added in Echidna hardfork (Policy extensions, Notary contract and etc.):

        {
            "id": -7,
            "key": "+f///xQi",
            "state": "Added",
            "value": "gJaYAA=="
        },
        {
            "id": -7,
            "key": "+f///xU=",
            "state": "Added",
            "value": "mDo="
        },
        {
            "id": -7,
            "key": "+f///xY=",
            "state": "Added",
            "value": "gBY="
        },
        {
            "id": -7,
            "key": "+f///xc=",
            "state": "Added",
            "value": "gBQg"
        },
        {
            "id": -1,
            "key": "/////wg77DUxEZu6123QRJILDebDGU/hwQ==",
            "state": "Added",
            "value": "QAUhAfYhACgUO+w1MRGbutdt0ESSCw3mwxlP4cEohk5FRjNuZW8tY29yZS12My4wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA4EEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0CdOC1CQQgoBk5vdGFyeUAASABAASgGTkVQLTI3QQJACEEFKAliYWxhbmNlT2ZAAUECKAdhY2NvdW50IQEUIQERIQAgAUEFKAxleHBpcmF0aW9uT2ZAAUECKAdhY2NvdW50IQEUIQERIQEHIAFBBSgZZ2V0TWF4Tm90VmFsaWRCZWZvcmVEZWx0YUAAIQERIQEOIAFBBSgQbG9ja0RlcG9zaXRVbnRpbEACQQIoB2FjY291bnQhARRBAigEdGlsbCEBESEBECEBFSAAQQUoDm9uTkVQMTdQYXltZW50QANBAigEZnJvbSEBFEECKAZhbW91bnQhARFBAigEZGF0YSEAIQL/ACEBHCAAQQUoGXNldE1heE5vdFZhbGlkQmVmb3JlRGVsdGFAAUECKAV2YWx1ZSEBESEC/wAhASMgAEEFKAZ2ZXJpZnlAAUECKAlzaWduYXR1cmUhARIhARAhASogAUEFKAh3aXRoZHJhd0ACQQIoBGZyb20hARRBAigCdG8hARQhARAhATEgAEAAQAFBAgAAQAAoBG51bGw="
        },
        {
            "id": -1,
            "key": "/////wz////2",
            "state": "Added",
            "value": "O+w1MRGbutdt0ESSCw3mwxlP4cE="
        },
        {
            "id": -10,
            "key": "9v///wo=",
            "state": "Added",
            "value": "jAA="
        },

Solution:

Since EnsureOmittedHardforks is always applied to configured hardfork values, this function ensures that all omitted hardfork heights are set to 0. We don't need additional checks anywhere and can rely on settings.Hardforks explicitly after that. If hardfork is not in settings.Hardforks then it is disabled.

Originally it was an intention of #2942 (comment), but somehow we missed this bug during review.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Node syncronisation and state dump comparison with the current mainnet

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Problem:

Consider a hardfork (Echidna in our case) that is:
 1. Explicitly ommitted from the node configuration;
 2. Given the fact that the set of previous hardforks are explicitly enabled
    in the node configuration.
The problem is that this hardfork is treated as "enabled from genesis"
by `IsInitializeBlock` function.

This problem is discovered when running N3 mainnet/testnet node with the default
configuration on the current master. This behaviour leads to unexected
Echidna-related changes being included into the node state starting from genesis
block. Here are some data:

Hardforks configuration that is used in config.json file (given this
config, Echidna should be treated as disabled):
```
    "Hardforks": {
      "HF_Aspidochelone": 210000,
      "HF_Basilisk": 2680000,
      "HF_Cockatrice": 3967000,
      "HF_Domovoi": 4144000
    },
```

Genesis block state difference between the current master and 3.7 version:
```
Processing directory BlockStorage_0
file BlockStorage_0/dump-block-0.json: block 0, changes length mismatch: 34 vs 41
```

In particular, there are 7 extra storage items stored in genesis. All
these changes relate to new functionality added in Echidna hardfork
(Policy extensions, Notary contract and etc.):
```
        {
            "id": -7,
            "key": "+f///xQi",
            "state": "Added",
            "value": "gJaYAA=="
        },
        {
            "id": -7,
            "key": "+f///xU=",
            "state": "Added",
            "value": "mDo="
        },
        {
            "id": -7,
            "key": "+f///xY=",
            "state": "Added",
            "value": "gBY="
        },
        {
            "id": -7,
            "key": "+f///xc=",
            "state": "Added",
            "value": "gBQg"
        },
        {
            "id": -1,
            "key": "/////wg77DUxEZu6123QRJILDebDGU/hwQ==",
            "state": "Added",
            "value": "QAUhAfYhACgUO+w1MRGbutdt0ESSCw3mwxlP4cEohk5FRjNuZW8tY29yZS12My4wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA4EEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0CdOC1CQQgoBk5vdGFyeUAASABAASgGTkVQLTI3QQJACEEFKAliYWxhbmNlT2ZAAUECKAdhY2NvdW50IQEUIQERIQAgAUEFKAxleHBpcmF0aW9uT2ZAAUECKAdhY2NvdW50IQEUIQERIQEHIAFBBSgZZ2V0TWF4Tm90VmFsaWRCZWZvcmVEZWx0YUAAIQERIQEOIAFBBSgQbG9ja0RlcG9zaXRVbnRpbEACQQIoB2FjY291bnQhARRBAigEdGlsbCEBESEBECEBFSAAQQUoDm9uTkVQMTdQYXltZW50QANBAigEZnJvbSEBFEECKAZhbW91bnQhARFBAigEZGF0YSEAIQL/ACEBHCAAQQUoGXNldE1heE5vdFZhbGlkQmVmb3JlRGVsdGFAAUECKAV2YWx1ZSEBESEC/wAhASMgAEEFKAZ2ZXJpZnlAAUECKAlzaWduYXR1cmUhARIhARAhASogAUEFKAh3aXRoZHJhd0ACQQIoBGZyb20hARRBAigCdG8hARQhARAhATEgAEAAQAFBAgAAQAAoBG51bGw="
        },
        {
            "id": -1,
            "key": "/////wz////2",
            "state": "Added",
            "value": "O+w1MRGbutdt0ESSCw3mwxlP4cE="
        },
        {
            "id": -10,
            "key": "9v///wo=",
            "state": "Added",
            "value": "jAA="
        },
```

Solution:

Since EnsureOmittedHardforks is always applied to configured hardfork
values, this function ensures that all omitted hardfork heights are set
to 0. We don't need additional checks anywhere and can rely on
settings.Hardforks explicitly after that. If hardfork is not in
settings.Hardforks then it is disabled.

Originaly it was an intention of #2942 (comment),
but somehow we missed this bug during review.

Signed-off-by: Anna Shaleva <[email protected]>
@Jim8y
Copy link
Contributor

Jim8y commented Apr 24, 2025

This is by all means should be an NEP or something, then the code. Previously we discussed a lot about how to treat a non-set hf, and existing code reflects that.. i am not against of change it if you think it is the best, but i call for an NEP to describe the hardfork rules.....

@NGDAdmin
Copy link
Collaborator

LGTM. Workflow is stuck.

@AnnaShaleva
Copy link
Member Author

Workflow is stuck.

It's not a problem of this PR, it looks like a problem of the workflow itself.

@shargon shargon merged commit b75d235 into master Apr 25, 2025
11 of 13 checks passed
@shargon shargon deleted the fix-hf-activation branch April 25, 2025 09:18
cschuchardt88 pushed a commit to cschuchardt88/neo that referenced this pull request Jun 8, 2025
Problem:

Consider a hardfork (Echidna in our case) that is:
 1. Explicitly ommitted from the node configuration;
 2. Given the fact that the set of previous hardforks are explicitly enabled
    in the node configuration.
The problem is that this hardfork is treated as "enabled from genesis"
by `IsInitializeBlock` function.

This problem is discovered when running N3 mainnet/testnet node with the default
configuration on the current master. This behaviour leads to unexected
Echidna-related changes being included into the node state starting from genesis
block. Here are some data:

Hardforks configuration that is used in config.json file (given this
config, Echidna should be treated as disabled):
```
    "Hardforks": {
      "HF_Aspidochelone": 210000,
      "HF_Basilisk": 2680000,
      "HF_Cockatrice": 3967000,
      "HF_Domovoi": 4144000
    },
```

Genesis block state difference between the current master and 3.7 version:
```
Processing directory BlockStorage_0
file BlockStorage_0/dump-block-0.json: block 0, changes length mismatch: 34 vs 41
```

In particular, there are 7 extra storage items stored in genesis. All
these changes relate to new functionality added in Echidna hardfork
(Policy extensions, Notary contract and etc.):
```
        {
            "id": -7,
            "key": "+f///xQi",
            "state": "Added",
            "value": "gJaYAA=="
        },
        {
            "id": -7,
            "key": "+f///xU=",
            "state": "Added",
            "value": "mDo="
        },
        {
            "id": -7,
            "key": "+f///xY=",
            "state": "Added",
            "value": "gBY="
        },
        {
            "id": -7,
            "key": "+f///xc=",
            "state": "Added",
            "value": "gBQg"
        },
        {
            "id": -1,
            "key": "/////wg77DUxEZu6123QRJILDebDGU/hwQ==",
            "state": "Added",
            "value": "QAUhAfYhACgUO+w1MRGbutdt0ESSCw3mwxlP4cEohk5FRjNuZW8tY29yZS12My4wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA4EEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0AQQRr3e2dAEEEa93tnQBBBGvd7Z0CdOC1CQQgoBk5vdGFyeUAASABAASgGTkVQLTI3QQJACEEFKAliYWxhbmNlT2ZAAUECKAdhY2NvdW50IQEUIQERIQAgAUEFKAxleHBpcmF0aW9uT2ZAAUECKAdhY2NvdW50IQEUIQERIQEHIAFBBSgZZ2V0TWF4Tm90VmFsaWRCZWZvcmVEZWx0YUAAIQERIQEOIAFBBSgQbG9ja0RlcG9zaXRVbnRpbEACQQIoB2FjY291bnQhARRBAigEdGlsbCEBESEBECEBFSAAQQUoDm9uTkVQMTdQYXltZW50QANBAigEZnJvbSEBFEECKAZhbW91bnQhARFBAigEZGF0YSEAIQL/ACEBHCAAQQUoGXNldE1heE5vdFZhbGlkQmVmb3JlRGVsdGFAAUECKAV2YWx1ZSEBESEC/wAhASMgAEEFKAZ2ZXJpZnlAAUECKAlzaWduYXR1cmUhARIhARAhASogAUEFKAh3aXRoZHJhd0ACQQIoBGZyb20hARRBAigCdG8hARQhARAhATEgAEAAQAFBAgAAQAAoBG51bGw="
        },
        {
            "id": -1,
            "key": "/////wz////2",
            "state": "Added",
            "value": "O+w1MRGbutdt0ESSCw3mwxlP4cE="
        },
        {
            "id": -10,
            "key": "9v///wo=",
            "state": "Added",
            "value": "jAA="
        },
```

Solution:

Since EnsureOmittedHardforks is always applied to configured hardfork
values, this function ensures that all omitted hardfork heights are set
to 0. We don't need additional checks anywhere and can rely on
settings.Hardforks explicitly after that. If hardfork is not in
settings.Hardforks then it is disabled.

Originaly it was an intention of neo-project#2942 (comment),
but somehow we missed this bug during review.

Signed-off-by: Anna Shaleva <[email protected]>
Co-authored-by: Jimmy <[email protected]>
Co-authored-by: Shargon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Used to tag confirmed bugs Core Ready to Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants