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

Adding reference to Nuget package of LevelDB.Standard #935

Closed
wants to merge 43 commits into from
Closed

Adding reference to Nuget package of LevelDB.Standard #935

wants to merge 43 commits into from

Conversation

eryeer
Copy link
Contributor

@eryeer eryeer commented Jul 19, 2019

Here is the reference change of LevelDB and the unit test of Persistence module that related to the change.

@codecov-io
Copy link

codecov-io commented Jul 19, 2019

Codecov Report

Merging #935 into master will decrease coverage by 2.36%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #935      +/-   ##
==========================================
- Coverage   60.83%   58.46%   -2.37%     
==========================================
  Files         195      186       -9     
  Lines       13333    13059     -274     
==========================================
- Hits         8111     7635     -476     
- Misses       5222     5424     +202
Impacted Files Coverage Δ
neo/IO/Caching/DataCache.cs 99.04% <0%> (-0.96%) ⬇️
neo/IO/Caching/MetaDataCache.cs 94.11% <0%> (-5.89%) ⬇️
neo/Persistence/LevelDB/LevelDBStore.cs 94.33% <100%> (+94.33%) ⬆️
neo/IO/Data/LevelDB/SliceBuilder.cs 54.76% <100%> (+54.76%) ⬆️
neo/Persistence/LevelDB/DbSnapshot.cs 100% <100%> (+100%) ⬆️
neo/IO/Data/LevelDB/Helper.cs 56.09% <50%> (+56.09%) ⬆️
neo/Persistence/LevelDB/DbCache.cs 89.28% <71.42%> (+89.28%) ⬆️
neo/Persistence/LevelDB/DbMetaDataCache.cs 85% <75%> (+85%) ⬆️
neo/Wallets/SQLite/UserWalletAccount.cs 0% <0%> (-100%) ⬇️
neo/Wallets/SQLite/VerificationContract.cs 0% <0%> (-100%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fd9239...a58bffa. Read the comment docs.

neo.UnitTests/TestUtils.cs Outdated Show resolved Hide resolved
neo/Persistence/LevelDB/DbCache.cs Show resolved Hide resolved
neo/Persistence/LevelDB/DbMetaDataCache.cs Show resolved Hide resolved
@@ -23,6 +23,7 @@
<ItemGroup>
<PackageReference Include="Akka" Version="1.3.11" />
<PackageReference Include="K4os.Compression.LZ4" Version="1.1.3" />
<PackageReference Include="LevelDB.Standard" Version="2.1.5" />
Copy link
Member

@shargon shargon Jul 19, 2019

Choose a reason for hiding this comment

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

I am scared about this, i love nugets, but this is a very humble nuget, so how could we are sure that the owner don't cheat us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikzhang , please please take a consideration about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shargon can't we "have our own"? Like "neo leveldb" in nuget?

Copy link
Member

Choose a reason for hiding this comment

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

This is a problem.

We also have a leveldb package on nuget, but it does not currently support .NET Core.

https://www.nuget.org/packages/LevelDB.Net/

Maybe we should modify this package and use it?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to fork it and modify it. Then will be owned by neo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to compile leveldb from google's repo and deploy it to work with neo-cli locally. So wouldn't it make more sense to start with the google's source than any other fork of the leveldb code?

Copy link
Contributor

@lock9 lock9 left a comment

Choose a reason for hiding this comment

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

Code looks good, thank you @eryeer!
I have to run prior to approving it.
Is this a neo 3 only? @shargon how do you plan to run this if neo 3 is not working with neo-cli? Or is it? I can't make it work with the latest code. Thanks

neo.UnitTests/Persistence/UT_Store.cs Outdated Show resolved Hide resolved
@lock9
Copy link
Contributor

lock9 commented Jul 19, 2019

I have tons of issues with the current leveldb. It takes more than 1 hour for a new developer to compile and make it work properly. I think this PR is great, I wish we could add this to NEO 2, but I don't think we have the time for that.

@devhawk
Copy link
Contributor

devhawk commented Jul 23, 2019

Reminder that C# can hide back doors too. Both Akka and K4os.Compression.LZ4 use unsafe code. Whatever process and policy used to monitor native dependencies like leveldb should also be used to monitor managed dependencies as well.

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 23, 2019

I love the unit tests on this PR, and I'm neutral regarding leveldb. If it's system managed, or nuget managed, it doesn't add extra security in my opinion.

If Neo servers are using ubuntu, with apt install libleveldb-dev, then version is probably v1.20, from 2017. Bitcoin uses v1.1, from 2011. Latest version is v1.22: https://github.com/google/leveldb
Precisely, current ubuntu uses this: 18 dez 28 2017 /usr/lib/x86_64-linux-gnu/libleveldb.so -> libleveldb.so.1.20.

@devhawk
Copy link
Contributor

devhawk commented Jul 23, 2019

Package management is clearly NOT a security system: https://haacked.com/archive/2019/06/11/package-mwanager-security/

@Tommo-L
Copy link
Contributor

Tommo-L commented Jul 31, 2019

Hi everyone, could we fork it, check and modify it, then release a neo version finally. It's easy for us to practice by this way. And it's more simpler for users by using nuget , they don't need to install the leveldb by themselves. 😂

@erikzhang
Copy link
Member

We are going to use RocksDB, I think. #966 😂

@eryeer
Copy link
Contributor Author

eryeer commented Nov 12, 2019

I will close this pr, since this change is replaced by RocksDB change.

@eryeer eryeer closed this Nov 12, 2019
@shargon
Copy link
Member

shargon commented Nov 12, 2019

This should be migrated to a plugin when Istorage is ready

@eryeer
Copy link
Contributor Author

eryeer commented Nov 12, 2019

This should be migrated to a plugin when Istorage is ready

Sure, I can reopen it or create a new PR then.

@shargon
Copy link
Member

shargon commented Nov 12, 2019

I am waiting with RocksDB opened as remember, when IStorage is ready, we can move it to neo-plugins

@shargon shargon reopened this Nov 12, 2019
@erikzhang erikzhang closed this Nov 25, 2019
@eryeer eryeer deleted the nuget_ref branch January 10, 2020 08:03
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
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.

8 participants