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

NEO: clear LastGasPerVote when voting for NULL, fix #2894 #3173

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

roman-khimov
Copy link
Contributor

It's a rather trivial fix, but it needs some substantial testing wrt account balances (just comparing the state won't be enough). @superboyiii.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit tests

This value won't be used in any way, so save some bytes of storage.

Signed-off-by: Roman Khimov <[email protected]>
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I remember this discussion some time ago. It seems to be ok.

@superboyiii
Copy link
Member

I will

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Seems good to me, just wait for @superboyiii

@superboyiii
Copy link
Member

Although it will surely make some storage changes, but checked NEO and GAS balance on all mainnet existed account, no problem. So it should be safe.

@shargon
Copy link
Member

shargon commented Mar 7, 2024

Although it will surely make some storage changes, but checked NEO and GAS balance on all mainnet existed account, no problem. So it should be safe.

Did you checked?

@superboyiii
Copy link
Member

superboyiii commented Mar 7, 2024

Although it will surely make some storage changes, but checked NEO and GAS balance on all mainnet existed account, no problem. So it should be safe.

Did you checked?

Yes, I checked. Many storage changes there. But the same event log(checked some of these) and the same NEO and GAS balance(all existed addresses).
78fbcefe59b9de71534b29303193711

For example, in a tx: 0x38091b803f794e50dcc10a9091becaf4f65d35d3ef9e71cfa90c7936af50757e on block 39632 of mainnet,
Original Storage Changes:

{
    "block": 39632,
    "size": 7,
    "storage": [
        {
            "state": "Changed",
            "key": "\u002Bv///xQXVU6kNdnigqZEh4tAVHW\u002BNwA2AQ==",
            "value": "QQEhBCg32Dw="
        },
        {
            "state": "Changed",
            "key": "\u002Bv///ws=",
            "value": "68Z3AaV6Eg=="
        },
        {
            "state": "Changed",
            "key": "\u002Bv///xSPX4PVGdD5t8CX0nYRsTB3r0935Q==",
            "value": "QQEhBbkBXAAW"
        },
        {
            "state": "Changed",
            "key": "\u002B////xQXVU6kNdnigqZEh4tAVHW\u002BNwA2AQ==",
            "value": "QQQhAlEEIQPQmgAAIQfI7lv90/cA"
        },
        {
            "state": "Changed",
            "key": "\u002B////wE=",
            "value": "Riw0AQ=="
        },
        {
            "state": "Changed",
            "key": "\u002B////yED/QTemD9OBMlimrPPyD9BvnQxuWv4UqkYc8OMqPc37iw=",
            "value": "QQIgASEDKV8C"
        },
        {
            "state": "Changed",
            "key": "\u002Bv///xRjslsjaXYnEvQP2GT6JumaYZgBIw==",
            "value": "QQEhBeYy2gAW"
        }
    ]
}

In this PR:

{
    "block": 39632,
    "size": 7,
    "storage": [
        {
            "state": "Changed",
            "key": "\u002Bv///xQXVU6kNdnigqZEh4tAVHW\u002BNwA2AQ==",
            "value": "QQEhBCg32Dw="
        },
        {
            "state": "Changed",
            "key": "\u002Bv///ws=",
            "value": "68Z3AaV6Eg=="
        },
        {
            "state": "Changed",
            "key": "\u002Bv///xSPX4PVGdD5t8CX0nYRsTB3r0935Q==",
            "value": "QQEhBbkBXAAW"
        },
        {
            "state": "Changed",
            "key": "\u002B////xQXVU6kNdnigqZEh4tAVHW\u002BNwA2AQ==",
            "value": "QQQhAlEEIQPQmgAAIQA="
        },
        {
            "state": "Changed",
            "key": "\u002B////wE=",
            "value": "Riw0AQ=="
        },
        {
            "state": "Changed",
            "key": "\u002B////yED/QTemD9OBMlimrPPyD9BvnQxuWv4UqkYc8OMqPc37iw=",
            "value": "QQIgASEDKV8C"
        },
        {
            "state": "Changed",
            "key": "\u002Bv///xRjslsjaXYnEvQP2GT6JumaYZgBIw==",
            "value": "QQEhBeYy2gAW"
        }
    ]
}

So the differ is between the storage of key \u002B////xQXVU6kNdnigqZEh4tAVHW\u002BNwA2AQ==
Original: QQQhAlEEIQPQmgAAIQfI7lv90/cA
VS
this PR: QQQhAlEEIQPQmgAAIQA=
1709805858089
But not sure what's stored in this key, could you help me check what's in this storage? @shargon

@roman-khimov
Copy link
Contributor Author

Exactly as expected, it's LastGasPerVote that's not going to be used (because account doesn't vote). So we store 0 (short) instead of something (more bytes).

@shargon shargon merged commit b05501a into neo-project:master Mar 7, 2024
6 checks passed
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 13, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 13, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 14, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 14, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 14, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 14, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 14, 2024
AliceInHunterland added a commit to nspcc-dev/neo-go that referenced this pull request Mar 15, 2024
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.

6 participants