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-RpcNep5Tracker, prevent it from leveldb bug. (2x) #139

Merged
merged 6 commits into from
Jan 8, 2020

Conversation

superboyiii
Copy link
Member

Fix-RpcNep5Tracker, prevent it from leveldb bug.

@erikzhang
Copy link
Member

What's this?

@superboyiii
Copy link
Member Author

@erikzhang We found if new a variable for ExtraGas evertime when execute ApplicationEngine.Run(), it will cause the bug of leveldb. Details please see here. neo-project/neo-node#472
We've already tested it on mainet, within this hotfix, chain number grows stablely. Without this, chain number grows rapidly which causes no space on disk. It's not a problem from our code but this could avoid causing leveldb bug.

@shargon
Copy link
Member

shargon commented Sep 30, 2019

This doesn't seems related to levelDB :S

@superboyiii
Copy link
Member Author

This doesn't seems related to levelDB :S

At first, I'm not sure. But when I saw the issue reproduced again, I just try to readjust it. And I test it several times, it truely avoids this issue. I resync the node within this version, it works stablely.

@shargon
Copy link
Member

shargon commented Sep 30, 2019

Maybe was a random error. Because this change seems that only change the scope of a variable not related with LevelDB.

@superboyiii
Copy link
Member Author

superboyiii commented Sep 30, 2019

@shargon Could you try to sync two nodes on mainet to the latest height on two different server with previous one and this one installed. You'll find the previous one will crash because of no space on disk and this one runs stablely. I think it's not a random error.

@erikzhang erikzhang closed this Nov 28, 2019
@erikzhang erikzhang deleted the Fix-Nep5Tracker branch November 28, 2019 12:47
@erikzhang erikzhang restored the Fix-Nep5Tracker branch November 28, 2019 13:27
@erikzhang erikzhang reopened this Nov 28, 2019
@@ -35,6 +35,7 @@ public class RpcNep5Tracker : Plugin, IPersistencePlugin, IRpcPlugin
private uint _maxResults;
private bool _shouldTrackNonStandardMintTokensEvent;
private Neo.IO.Data.LevelDB.Snapshot _levelDbSnapshot;
private Fixed8 maxGas = Fixed8.FromDecimal(100000000m);
Copy link
Member

Choose a reason for hiding this comment

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

private const?

@vncoelho
Copy link
Member

vncoelho commented Nov 28, 2019

I am not sure, @superboyiii, however, your tests are usually always right! ahuehaeahuea

If it solves something I think you should proceed because there is not a notorious danger on this.

Limit maxGas
@superboyiii
Copy link
Member Author

superboyiii commented Nov 29, 2019

@vncoelho Hey, Vitor, my man. Thanks for your trust. This story begins here: google/leveldb#320.
Details can be seem here: neo-project/neo-node#472
What I did was tested to prevent neo-cli from this issue, that's an accidental chance when I found that changing code to this way could avoid this issue. Could you help me to double check?
You need to do these:

  1. Sync a node with master-2.x's version, build RpcNep5Tracker, and put it into Plugins folder. Try to sync to the latest height with offline package: https://sync.ngd.network/. After about ten hours, you'll find your disk has few space left.
  2. Build this version and do the same things again. After several hours, you'll find everything is normal and your node has already sync to the latest height.

@@ -35,6 +35,7 @@ public class RpcNep5Tracker : Plugin, IPersistencePlugin, IRpcPlugin
private uint _maxResults;
private bool _shouldTrackNonStandardMintTokensEvent;
private Neo.IO.Data.LevelDB.Snapshot _levelDbSnapshot;
private Fixed8 maxGas = Fixed8.FromDecimal(1m);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private Fixed8 maxGas = Fixed8.FromDecimal(1m);
private static readonly Fixed8 maxGas = Fixed8.FromDecimal(1m);

@erikzhang
Copy link
Member

Please test it.

@vncoelho
Copy link
Member

Surely, @superboyiii, my pleasure.
It is always great to hear the history behind, these ideas should be valued because of that, it is always not easy to find any problem. But, when we find we are happy ;D

Give me some couple of days.
Can you describe 1. again, please?
I need to sync the node first and then include the plugin? Can not just include the plugin and sync with bootstrap?

@shargon
Copy link
Member

shargon commented Nov 29, 2019

We can upgrade leveldb in order to prevent future issues

@superboyiii
Copy link
Member Author

superboyiii commented Nov 29, 2019

@shargon Yes, that's the best solution. Please take a look at here. neo-project/neo-node#472 @Ashuaidehao has already build the latest leveldb library, and we've test it on three different OS: Win10,Ubuntu 16.04 and MacOS. We've made regression functional and rpc test, all pass. The final step we have to do is compare it with previous data to ensure it's compatible. @nicolegys is doing this, it will take several days, let's wait for it.

@superboyiii
Copy link
Member Author

Surely, @superboyiii, my pleasure.
It is always great to hear the history behind, these ideas should be valued because of that, it is always not easy to find any problem. But, when we find we are happy ;D

Give me some couple of days.
Can you describe 1. again, please?
I need to sync the node first and then include the plugin? Can not just include the plugin and sync with bootstrap?

Bootstrap is available. Recommend to use bootstrap.

Fix
@vncoelho vncoelho changed the title Fix-RpcNep5Tracker, prevent it from leveldb bug. Fix-RpcNep5Tracker, prevent it from leveldb bug. (2x) Dec 18, 2019
vncoelho
vncoelho previously approved these changes Jan 7, 2020
@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2020

@superboyiii, can we merge it?

I did not had time to test. However, the change is trivial and does not produces any danger.

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2020

If merged, perhaps we need to port to master branch.

@erikzhang erikzhang merged commit 30ddf5c into master-2.x Jan 8, 2020
@erikzhang erikzhang deleted the Fix-Nep5Tracker branch January 8, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants