Transaction pinning: #1599 #1632

ovanwijk · 2019-09-30T22:50:33Z

Description

Allows IOTA Nodes to use a separate data storage in order to pinTransactions through the webapi.
A pinned transaction is a transaction that will not be deleted during snapshotting. In order to not interfere with the current implementation of the persistence layer and the snapshotting code this solution uses a separate database file to store permanently stored transactions.

It allows for arbitrary transaction storages through 'pinTransactionsTrytes' and it allows for copying transactions by 'pinTransactionHashes'.

Unpinning is also possible. This will remove the transactions from the permanent storage provider.

'isPinned' as api is introduced to check if transactions are pinned or not.

Pinned transactions can be accessed through the normal api calls like 'findTransactions' and 'getTrytes'.

Example: in order to pin all transactions from an address just call findTransactions(address) and then use the result in the 'pinTransactionHashes'.

The RocksDBPPPimpl.java code also offers inplace replacement code for updating indexes for deleting data. This could be used 1:1 for solving #1307

Fixes # (issue)
#1599

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Adds a set of tests that use the database file to save, delete and retrieve data.

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

…s based on the current TxModel and rocksdb persistence layer.

- Introduced an interface for permanent storage. - Added a permanent storage PersitenceProvider - Added mergeTwo() function to genericly handle class independant merging. - Changed Tangle.load() to handle multiple persistence providers and handle with mergable results (like searching addresses in two persistence providers) - Tangle.load() didn't handle this because persistenceProvider.get never returned null, only a new (empty) instance of the requested object. The bytes() wasnt uniform in implementation and would for example throw an exception on Milestone, the MilestoneViewModel did the null check. Therefore the introduction of isEmpty() for Persistables.

- Added functions to the tangle to handle permanent storage - Added API's: * pinTransactionHashes (hashes:[]) * pinTransactionTrytes (trytes:[]) * increaseTransactionCount (hashes:[]) * decreaseTransactionCount (hashes:[]) * getTransactionsCount (hashes:[]) - or others to test this branch: set default settings for permanent storage

…isPinned is left.

# Conflicts: # src/main/java/com/iota/iri/Iota.java # src/main/java/com/iota/iri/model/StateDiff.java # src/main/java/com/iota/iri/model/persistables/Hashes.java # src/main/java/com/iota/iri/model/persistables/Milestone.java # src/main/java/com/iota/iri/model/persistables/Transaction.java # src/main/java/com/iota/iri/storage/Persistable.java # src/main/java/com/iota/iri/storage/Tangle.java

GalRogozinski

This is an initial review.
Overall it looks good :-)

After you fix my few nits I will take another look.
Talk to me in private if you are being blocked

GalRogozinski · 2019-11-07T10:03:55Z

src/main/java/com/iota/iri/Iota.java

+                    break;
+                }
+                default: {
+                    throw new NotImplementedException("No such database type.");


I prefer to use UnsupportedOperationException, just because it doesn't have a dependency on a 3rd party library

src/main/java/com/iota/iri/service/API.java

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

Co-Authored-By: Gal Rogozinski <[email protected]>

GalRogozinski

Looked at it a bit more now
The design change I am asking for is rather important from my POV because it will save us work later I believe

GalRogozinski · 2019-11-17T11:20:23Z

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

+        if (dbResult != null && dbResult.length > 0) {
+            return new TransactionViewModel(TransactionViewModel.trits(dbResult), Hash.NULL_HASH);
+        }
+        return null;


Currently, we always return a new empty transaction object, which is obviously bad.
In #1591 we said we will return an instance of an immutable static dummy object instead. I prefer that you do that.

If you prefer to return null, please explain why and make sure the code is impervious to null pointer exceptions

Making an empty static object would be fine with me as well but I think that is outside of the scope for this pull request. the function getTransaction is part of the PermanentPersistenceProvider and not of the PersistenceProvider.

I chose null because it is more explicit than an empty object. In the get function of the PersistenceProvider interface this is explicitly handled.

It is a possible way to go about it.

However, the reason I preferred to go about an empty object is because then you have to remember to always do null checks. If you forget (like in safeDeleteTransaction :-P) you later have to go back and add it...

The thing is that probably the PermanentPersistenceProvider and PersistenceProvider will have to be similar in this sense eventually, so might as well get it right from the beginning :-)

I only asked you to add this empty object in PermanentPersistenceProvider and not in the old code. If you feel strongly against this then just know:

You will have to add lots of null checks ;-)

It will just make things messier later

GalRogozinski · 2019-11-17T11:21:24Z

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

+    @VisibleForTesting
+    void safeDeleteTransaction(WriteBatch writeBatch, Hash key) throws Exception {
+        byte[] keyBytes = key.bytes();
+        TransactionViewModel tx = getTransaction(key);


what happens if this returns null?
See my other comment and #1591

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

GalRogozinski · 2019-11-17T12:56:22Z

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

+    void addToIndex(WriteBatch writeBatch, ColumnFamilyHandle column, Indexable key, Indexable indexValue)
+            throws Exception {
+        byte[] indexBytes = indexValue.bytes();
+        byte[] dbResult = db.get(column, key.bytes());


shouldn't it be better to use the exists() method that checks mayExists first?

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

GalRogozinski · 2019-11-17T15:31:34Z

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

+/***
+ * Implements the retrieval functions of a the normal persistence provider and the permanent persistence provider
+ */
+public class RocksDBPPPImpl implements PermanentPersistenceProvider, PersistenceProvider {


Now having a deeper look here, I would like to offer a different design :-)

Currently, it is possible with the current design it is theoretically possible to implement a new PersistanceProvider that will replace RocksDb. You have actually attempted to do it yourself :-)

Here you tightly coupled RocksDb with the PermenantPersistanceProvider...
This means that if we ever want to replace RocksDB it will be much harder (there are actually thoughts about doing this now - #1661).

Also I have noticed lots of evil copy paste... Here we are believers in DRY (don't repeat yourself).

So maybe you can create PermenantPersistanceProvider that contains a RocksDbPersistanceProvider?
This way if we write a new PersistnaceProvider, switching will be a breeze?

It will also adhere better to the single responsibility principle that we have a class that only deals with wrapping the RocksDb api and a class that deals with the logic of your task :-)

# Conflicts: # src/main/java/com/iota/iri/Iota.java

GalRogozinski · 2019-12-26T13:27:52Z

Hey @ovanwijk, you want another review?
I think I have comments you didn't tend to, which is why this is being ignored

# Conflicts: # src/main/java/com/iota/iri/Iota.java

GalRogozinski · 2020-01-20T10:39:03Z

src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

+        return found;
+    }
+
+    private void initDB(String path, String logPath) throws Exception {


Issue found: The method 'initDB(String, String)' has a cyclomatic complexity of 10.

ovanwijk added 13 commits August 7, 2019 14:47

Counted transaction model for selective permanode storage capabilitie…

0b8a6bd

…s based on the current TxModel and rocksdb persistence layer.

Merge branch 'dev' into SPstorage

18d6ced

Removed the countered transaction model, just pinning, unpinning and …

c28bbaa

…isPinned is left.

Renaming and comments

e27f1e7

Fix for RocksDBPPPImpl to handle integer indexes.

c989470

Name changes and comments

be9fd59

Added boolean return values for transactionPinning

5d5d463

Revert test code to original code

fe988f7

Space removal

4b21d42

Braces fix

322764a

iotaledger deleted a comment Sep 30, 2019

iotaledger deleted a comment Oct 1, 2019

ovanwijk added 3 commits October 1, 2019 11:51

Import hell, why is this better then .* ?

68680f8

Constructor comment and boolean response comment

74f86e7

Merge branch 'dev' into SPstorage

2a8330d

GalRogozinski suggested changes Nov 7, 2019

View reviewed changes

ovanwijk and others added 8 commits November 10, 2019 17:16

Update src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

0c96323

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

734fa04

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/service/API.java

5d467d9

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/service/API.java

9dee4cd

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/service/API.java

e6288e9

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

06d35b8

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/storage/rocksDB/RocksDBPPPImpl.java

bf0fb20

Co-Authored-By: Gal Rogozinski <[email protected]>

Update src/main/java/com/iota/iri/service/API.java

7faad38

Co-Authored-By: Gal Rogozinski <[email protected]>

GalRogozinski suggested changes Nov 17, 2019

View reviewed changes

Merge branch 'dev' into SPstorage

3363209

# Conflicts: # src/main/java/com/iota/iri/Iota.java

GalRogozinski mentioned this pull request Nov 18, 2019

Fix: Zmq documentation #1665

Merged

1 task

ovanwijk added 2 commits November 19, 2019 10:04

Commit and merge

5abc785

Null fix and mayExist fix

e5a8f16

ovanwijk added 2 commits January 20, 2020 11:32

Fix API check

97d5ec0

Merge branch 'dev' into SPstorage

920e60b

# Conflicts: # src/main/java/com/iota/iri/Iota.java

GalRogozinski reviewed Jan 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction pinning: #1599 #1632

Transaction pinning: #1599 #1632

ovanwijk commented Sep 30, 2019 •

edited

Loading

GalRogozinski left a comment

GalRogozinski Nov 7, 2019

GalRogozinski left a comment

GalRogozinski Nov 17, 2019

ovanwijk Nov 19, 2019

GalRogozinski Nov 20, 2019

GalRogozinski Nov 17, 2019

GalRogozinski Nov 17, 2019

GalRogozinski Nov 17, 2019

GalRogozinski commented Dec 26, 2019

GalRogozinski Jan 20, 2020

Transaction pinning: #1599 #1632

Are you sure you want to change the base?

Transaction pinning: #1599 #1632

Conversation

ovanwijk commented Sep 30, 2019 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

GalRogozinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski commented Dec 26, 2019

Choose a reason for hiding this comment

ovanwijk commented Sep 30, 2019 •

edited

Loading