Preperation to support different database backends

[merl111]

What current issue(s) does this address, or what feature is it adding?

preperation for #896
closes #895
closes #894

How did you solve this problem?

The whole database access has been reworked and decoupled. Business logic which has been part of LevelDBBlockchain has been moved to the Blockchain class. A factory to dynamically generate the database class has been introduced. The class returned by the factory handles write read delete access to the database, therefore it's completely decoupled from the blockchain business logic. Same thing was done for the NotificationDB.

How did you make sure your solution works?
Synced full node.
Bootstrapped node.
All unit tests pass.

Are there any special changes in the code that we should be aware of?
The whole business logic from LevelDBBlockchain has been moved to the Blockchain class.

Please check the following, if applicable:

• Did you add any tests?
• Did you run make lint?
• Did you run make test?
• Are you making a PR to a feature branch or development rather than master?
• Did you add an entry to CHANGELOG.rst? (if not, please do)

[coveralls]

Coverage increased (+0.8%) to 85.912% when pulling bc6883e on merl111:alt-db-support into a90947f on CityOfZion:development.

[merl111]

I would also like to reference np-rocksdb which is based on alt-db-support and implements RocksDB as an alternative database backend. It's not fully tested yet though, all the unit tests pass ( fixtures are migrated to RocksDB ) but I have not yet synced a full node.

[ixje]

Given that we've already discussed quite some on Discord there's mostly some nitpicks on typo's/documentation and then 2 actual items that need some work.

[ixje]

One more things I encountered when running your LevelDB implementation

File "./neo/bin/prompt.py", line 304, in main
    blockchain = Blockchain(DBFactory.getBlockchainDB())
  File "/Users/erik/Documents/code/test-issues/merl/neo/Core/Blockchain.py", line 192, in __init__
    with self._db.openIter(DBProperties(prefix='')) as iterator:
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/contextlib.py", line 112, in __enter__
    return next(self.gen)
  File "/Users/erik/Documents/code/test-issues/merl/neo/Storage/Implementation/LevelDB/LevelDBImpl.py", line 132, in openIter
    include_key=properties.include_key)
  File "plyvel/_plyvel.pyx", line 362, in plyvel._plyvel.DB.iterator
TypeError: Argument 'prefix' has incorrect type (expected bytes, got str)

[ixje]

Another thing that came to mind; we should provide a set of test vectors that new implementations can run against.

For example my SQLite test implementation doesn't fail until it hits some block in which it requests a TX. It can actually get data from the DB, but the format is wrong. This particular code (of GetTransaction) was once LevelDB specific and is now generic in Blockchain.py. That is fine but it expects a certain format

neo-python/neo/Implementations/Blockchains/LevelDB/LevelDBBlockchain.py

Lines 387 to 392 in fe90f62

	out = self._db.get(DBPrefix.DATA_Transaction + hash)
	if out is not None:
	out = bytearray(out)
	height = int.from_bytes(out[:4], 'little')
	out = out[4:]
	outhex = binascii.unhexlify(out)

Meaning it expects a bytearray where the first 4 bytes are escaped, and the remainder are not 🙈
e.g.

b'\x01\x02\x03\x04aabbccdd`

[merl111]

One more things I encountered when running your LevelDB implementation

Fixed.

[merl111]

Another thing that came to mind; we should provide a set of test vectors that new implementations can run against.

I have written a few generic tests during the implementation of RocksDB, I added those to the alt-db-support branch, but I am not sure if that's exactly what you are asking for.

For example my SQLite test implementation doesn't fail until it hits some block in which it requests a TX. It can actually get data from the DB, but the format is wrong. This particular code (of GetTransaction) was once LevelDB specific and is now generic in Blockchain.py. That is fine but it expects a certain format.

I think I am missing something here.

A transaction is stored in Persist:

https://github.com/merl111/neo-python/blob/c912b2be80fc80887e73f403cb9584aa090e993e/neo/Core/Blockchain.py#L974

the value is created from block.IndexBytes() + tx.ToArray() means the first 4 bytes are the IndexBytes, so in my opinion the code in GetTransaction is not LevelDB specific, but specific to what is inserted when the tx is written to the database.

I'll hit you up on discord, to discuss this.

[ixje]

the value is created from block.IndexBytes() + tx.ToArray() means the first 4 bytes are the IndexBytes, so in my opinion the code in GetTransaction is not LevelDB specific, but specific to what is inserted when the tx is written to the database.

I'll hit you up on discord, to discuss this.

True, it's not specific anymore. It's just that argument type and format is derived from what was once necessary for leveldb. For example for sqlite3 the values need to be strings and queries return strings. However all methods in neo-python currently operate on b'' bytearrays. Because that's what was once returned by LevelDB/Plyvel.

[merl111]

Ok, I understand, that should be covered in the docstrings for get now. Bytestrings mit have some advantages over strings in some cases, so for now I would just check if get really returns a bytearray, would that be sufficient?

[ixje]

This branch doesn't seem to sync. Probably because it can't retrieve data

neo> show block 0
[I 190521 10:50:51 Blockchain:674] Could not get block 'NoneType' object is not iterable
Could not locate block 0
neo> show header 0
Could not locate header 0

[ixje]

I did a quick test run just now and so far looks good 👍 .

I'm gonna use your branch to do the audit with as the audit also includes storage comparison. Otherwise I'd have to do that part twice, which doesn't make sense. This does mean it can take a couple of days before it gets merged (fyi)

[merl111]

Ok perfect, thank you for the response.

[ixje]

As mentioned previously a quick np-prompt suggested it was working as intended. Today I tried to import blocks for the auditing purpose and I ran into a couple of issues

1. np-import doesn't work because the script was not converted to use the new db layer. (probably because it's not covered by the tests
2. Trying to update that I quickly ran into a circular import error for neo.Core.Blockchain import Blockchain import statements . After some dirty fixes I could get it to run, but within importing a couple thousand blocks I received errors like the following:

Traceback (most recent call last):
  File "/home/erik/merl/neo/Core/Blockchain.py", line 1100, in Persist
    service.ExecutionCompleted(engine, success)
  File "/home/erik/merl/neo/SmartContract/StateMachine.py", line 219, in ExecutionCompleted
    super(StateMachine, self).ExecutionCompleted(engine, success, error)
  File "/home/erik/merl/neo/SmartContract/StateReader.py", line 130, in ExecutionCompleted
    tx_hash = engine.ScriptContainer.Hash
AttributeError: 'bytes' object has no attribute 'Hash'

3. Having looked closer while keeping performance in mind I believe we should not use the static ApplicationEngine.Run() in Persist(). Reason being is that for each invocation TX that's executed it will try to import the required modules, will setup all required DBInterface's, then execute. In the old setup this was done just once per Block. Imagine a block with 50+ InvocationTransactions and we're going to lose quite some speed.

My suggestion would be for you to update np-import (== ./neo/bin/import_blocks.py) to support the new DB layer, then make sure you can import e.g. the first 100K blocks.

[ixje]

I didn't notice this before, but the --datadir flag isn't working for np-prompt.

(venv) erik@erik-linux:~/merl$ np-prompt --datadir test_storage/
Maxpeers set to  5
[I 190612 08:07:54 LevelDBImpl:43] Created DB at /home/erik/.neopython/Chains/SC234 
[I 190612 08:07:55 LevelDBImpl:43] Created DB at /home/erik/merl/test_storage/Chains/Test_Notif 
[I 190612 08:07:55 NotificationDB:75] Created Notification DB At /home/erik/merl/test_storage/Chains/Test_Notif 
[2019-06-12 08:07:55.454651] Running P2P network on localhost 20333
NEO cli. Type 'help' to get started


neo>                                                                            

Notice how I specified a local folder but it still put it in the default ~/.neopython/ location.

Another thing is the double Test_Notif location message. Although printed from different classes a double message adds no extra value. Please remove one of them.

Other than that all looks ok. I can import blocks, gather blocks over P2P, query blocks/headers/tx's, still create new tx's (sending assets) and have them stored properly.

When merging the dev branch probably the only PR that will cause a conflict is https://github.com/CityOfZion/neo-python/pull/965/files (because it touched the LevelDB class directly)

[ixje]

500 points

[lllwvlvwlll]

https://neoscan.io/transaction/26ef4b7c07219eadd7d74ce0eff4c3a43271921d263db6a400cb075d67c39cb0