reconsider PR #2841

[vang1ong7ang]

PR #2841 introduced an optimization for the storage of VoterRewardPerCommittee

BUT

This will fork the chain

_Originally posted by @shargon in https://github.com/neo-project/neo/pull/2841#discussion_r1065605611_

That's why I'm strongly against this PR.

However, I believe the benefits it brings are not worth it (LESS THAN 10% storage/synctime saving).

Mainnet fullstate leveldb sync compare:

branch	sync time(hour:min)	MPT data size	block data size
3.5.0	10:16	50G	2.4G
3.5.0	10:21	50G	2.4G
this patch	10:01	46G	2.3G
this patch	10:09	46G	2.3G

Originally posted by @ZhangTao1596 in https://github.com/neo-project/neo/issues/2841#issuecomment-1427232281

2841 changed the behaviors of public methods of native contract NeoToken, which are the most fundamental interfaces on the blockchain. I believe these interfaces should be stable enough and not easily changed AND forward compatibility SHOULD BE ALWAYS GUARANTEED.

I have to correct the view of @roman-khimov: methods like UnclaimedGAS are critical interfaces to NEO staking applications like NeoBurger and NeoCompounder. The impact will then be transmitted to defi, nft markets and even other applications.

... but we might as well not do that, I don't see a lot of uses for this data). But anything else will just work: balances, transfer data, GAS calculations, etc. So it looks more like a regular DB resync to me.

_Originally posted by @roman-khimov in https://github.com/neo-project/neo/pull/2841#discussion_r1066259847_

---

I propose to:

• pause mainnet upgrade of version 3.6.0
• replay historical transactions in both versions to avoid state conflict
• revert PR #2841 and proceed it after version control mechanism is introduced in native contracts

[dusmart]

If we require the 2nd parameter of unclaimedGAS to be a specific number as that PR introduced, why not delete the second parameter and leave all those DEFI projects relying on this cry alone.

[roman-khimov]

2841 was like half a year ago, it'd be nice to discuss it back then. But:

• getAccountState return value is extended, adapting to that is not hard (although I've said even back then that the public NEO interface could be preserved)
• unclaimedGas can not work with arbitrary end, it was pretending to be working, but the number you got there was only valid for holders, for voters it could be arbitrarily different from the truth. That's something worth taking into account in bNEO/cNEO anyway.

Take a look at nspcc-dev/neo-go#2892 as well, in some scenarios it's much more than 10% improvement. And it's not just raw data size/sync speed that we improve with #2841. Remember the original #2815 problem, old scheme limits some use cases just because NEO contract has too much junk stored in it.

[vang1ong7ang]

• getAccountState return value is extended, adapting to that is not hard (although I've said even back then that the public NEO interface could be preserved)

adapting is not hard, but it should be SERIOUS to treat interfaces changes.

imaging someone was writing TO, HOLD = getAccountState.FIRST, getAccountState.LAST, if the behavior of the interface SILENTLY changes, it's a disaster for the application. (yes, that's what bneo actually do, FORTUNATELY, I HAVE TO SAY FORTUNATELY, I noticed it, otherwise, you know what will happen)

I believe getAccountStateV2 should be always introduced unless getAccountState(V1) cause bug.

should never speculate how others' work is depend on this interface

[dusmart]

As a NEO blockchain developer, I always love to change things without writing it in a new implementation and change it silently. Even if I know it’s a hardfork and a hardfork tag is introduced, I still want to ignore the tag and delete old implementation as well as change all the code like this PR did.

As a dAPP developer, I would definitely want all the existing things remain the same. And breaking change seems good to me compared to silent change. The two parameter unclaimedGAS cheated me at first. If the dAPP didn’t run a testnet version, we’ll have to face the problem suddenly when 3.6.0 is online.

[roman-khimov]

it should be SERIOUS to treat interfaces changes

And they're treated seriously, at least you can see all of these things in the discussions. But we only have the chain itself to verify things against (remember, it's a contract API) and this never affected any of the existing transactions. The discussion went on for quite some time and was absolutely public. And I agree funcV2 would be a better thing. Yet at the same time there were different opinions and some consensus had to be reached there. If this particular use case was added into discussion back then, likely we'd make a bit different choices.

At the same time

TO, HOLD = getAccountState.FIRST, getAccountState.LAST

is somewhat strange for me to see as well. It's a structure and every field there has an index, so it's not a good access pattern (but it's a different problem, I agree).

The two parameter unclaimedGAS cheated me at first.

This was also discussed as you've probably seen, because the initial implementation did remove the second parameter. But this was ruled out as a breaking API change. Now, if it's really important for some apps to have this rough estimation from the contract, this particular behavior could probably be reinstantiated without the storage scheme revert.

we’ll have to face the problem suddenly when 3.6.0 is online

Well, have a look at #2810 then.

But we still can have a 3.6.1 addressing API changes (storage scheme can stay the same, it's all about API) if needed.

[roman-khimov]

BTW, back when #2841 was discussed we didn't have Basilisk HF, so as you can see the logic in this PR is not tied to the HF. We can make it be so, at least keeping things more compatible before HF actually happens. And some application adaption can be expected of a hardfork.

[zk524]

BTW, back when #2841 was discussed we didn't have Basilisk HF, so as you can see the logic in this PR is not tied to the HF. We can make it be so, at least keeping things more compatible before HF actually happens. And some application adaption can be expected of a hardfork.

As a neo edge developer and user, it's really frustrating to have to resynchronize the blockchain every time there's an upgrade, especially considering that the current mainnet offline package is only updated until April and now it's already September. Not to mention the occasional problems encountered during synchronization, it really drives me crazy. Could we make fewer changes that break compatibility and focus more on meaningful upgrades instead? Sometimes I wonder, while Ethereum has reached $2,000, neo, which was at a similar stage, is barely above $10.

[vang1ong7ang]

okay. it sounds reasonable. I'm going to close this issue

[roman-khimov]

Irrespective of this particular issue, please, please, please submit bugs, file issues, complain and participate in ongoing discussions. We don't know every use case, we don't know all the problems, sometimes we know of them, but we have established practices that can be reviewed and revised. Either way we need to communicate to deliver better product for everyone.

[vang1ong7ang]

please understand clearly: @roman-khimov

if some bad guys are observing the discussion on github NOW, he CAN make use of the hardfork of 3.5.0 and 3.6.0 NOW by pining a conflict state into a transaction and deposit his funds into binance in the same transaction and wait for the return back of his money when the upgrade comes

[cschuchardt88]

I know the reason we resync the blockchain, But the state doesn't change for the ledger, unless forked. Maybe we should incorporate exports for plugin storage. And if you don't trust the back up, well resync your node. What i'm saying is improve that backup we use for syncing (chain.0.acc.zip); as in all ready processed data. Yes, i know people could have different Storage plugins and you can't have a backup for everything. Another note I think plugin system needs to be reworked as well, but that's a different story altogether.

[vang1ong7ang]

But the state doesn't change for the ledger

@cschuchardt88 state is possible to change because of #2841 . This is not a compatible change

[cschuchardt88]

What I mean, is in context; once the node processes a block, it's done and over with, that's in the pass now...

[vang1ong7ang]

yes it should be. but actually when fork is introduced, the storage will be rewrite and even the state will be revert

[vang1ong7ang]

if resync is necessary, then the state in the pass is possible to be revert.

otherwise, state differs from new running nodes and existing nodes.

[cschuchardt88]

maybe a new approach to sync/storage is necessary, like a mix of BitTorrent and Git and they both open source too. 😄

[vang1ong7ang]

as you said, the historical state / storage should be ensured and that's the purpose of basilisk HF.

try something like NEO.unclaimedGAS(blockheight + 8); NEO.transfer(me, binance, NEO.balanceOf(me)) in a single transaction in version 3.5.0 and then upgrade it to 3.6.0, the money will go back. because the transaction is valid in v3.5.0 but when you resync it in v3.6.0, it will revert.

[cschuchardt88]

@vang1ong7ang Not saying my system or idea is perfect for every scenario. Its like a car it has wheels, engine, takses gas...But there are many different cars. But the Foundation is modular and can easily be changed or removed and still have it be a car...

@zk524 Sometimes I wonder, while Ethereum has reached $2,000, neo, which was at a similar stage, is barely above $10.

Neo tries to reinvent the wheel which makes it hard or in most cases incapable (In code, not technology) with everything but itself; not including interop service.

[vang1ong7ang]

@cschuchardt88 I agree with your ideas of new approach to sync/storage.

but the issue here is talking about the possible fork. let's focus on the objective risk and ignore all the jokes and complains.

[vang1ong7ang]

replaced and maintained by #2910

[roman-khimov]

state is possible to change because of #2841

Likely more because of #1526.