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

Spent Addresses should be persisted in a seperate db instance #1263

Merged
merged 9 commits into from Jan 10, 2019

Conversation

Projects
None yet
4 participants
@GalRogozinski
Copy link
Member

GalRogozinski commented Jan 9, 2019

Description

Persists spent addresses to a separate db instance

Fixes #1262

Type of change

  • Enhancement (a non-breaking change which adds functionality)

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 added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

GalRogozinski added some commits Jan 7, 2019

@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Jan 9, 2019

@DyrellC

Please test this.
Look for the following:

  1. false positives
  2. confirmed spent from addresses before/after snapshot
  3. unconfirmed spent from addresses before/after snapshot
  4. confirmed spent from addresses before/after pruning
  5. unconfirmed spent from addresses before/after pruning

@GalRogozinski GalRogozinski requested review from alon-e , hmoog and kwek20 Jan 9, 2019

@kwek20
Copy link
Member

kwek20 left a comment

Im not sure about add/save change, Thats kind of implementation specific. But it works for me, i dont think we will quickly switch to a non-saveable implementation :)

Show resolved Hide resolved ...com/iota/iri/service/spentaddresses/impl/SpentAddressesProviderImpl.java Outdated
@@ -71,6 +75,18 @@
*/
public class Iota {
private static final Logger log = LoggerFactory.getLogger(Iota.class);
public static final HashMap<String, Class<? extends Persistable>> COLUMN_FAMILIES =

This comment has been minimized.

@kwek20

kwek20 Jan 9, 2019

Member

This seems off compared to the rest of the class.

I would rather see 2 separate enums for this instead of a "random" map in the main class.
The main class is now imported on multiple places just for these 2 family maps.

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 9, 2019

Member

The main class should only be used to inject dependencies.
I definitely agree with you that this detail doesn't belong here. It should belong to the Tangle class that needs to define what column families do we use.

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 9, 2019

Member

Another problem is that I used HashMap instead of Map.

Also I didn't use an immutable type...
In java 9 we will get Map.of()
I think I will leave it as it is now, and push soon for an upgrade to Java 11 :-)

@@ -85,26 +76,16 @@ private void readPreviousEpochsSpentAddresses() {
readSpentAddressesFromStream(
SpentAddressesProviderImpl.class.getResourceAsStream(previousEpochsSpentAddressesFile));
} catch (SpentAddressesException e) {
log.error("failed to read spent addresses from " + previousEpochsSpentAddressesFile, e);
log.error("Failed to read spent addresses from " + previousEpochsSpentAddressesFile, e);

This comment has been minimized.

@kwek20

kwek20 Jan 9, 2019

Member

We might want to throw the error here, and not catch it in the constructor. Pretty important if were missing/failing to read this file

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 9, 2019

Member

It was like this before. I didn't catch Hans on that...
Good job :-)


private RocksDB db;
// DBOptions is only used in initDB(). However, it is closeable - so we keep a reference for shutdown.
private DBOptions options;
private BloomFilter bloomFilter;
private boolean available;

public RocksDBPersistenceProvider(String dbPath, String logPath, int cacheSize) {
public RocksDBPersistenceProvider(String dbPath, String logPath, int cacheSize,
Map<String, Class<? extends Persistable>> columnFamilies,

This comment has been minimized.

@kwek20

kwek20 Jan 9, 2019

Member

.. Continue from Iota.java:
And then replace this part by reading all enum fields & class value, which we add to the map

GalRogozinski added some commits Jan 9, 2019

@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Jan 9, 2019

@kwek20

Im not sure about add/save change, Thats kind of implementation specific.
Provider is a confusing name. It should be named Repository. From here you understand that the point of this class is to always persist data.

@DyrellC

This comment has been minimized.

Copy link
Contributor

DyrellC commented Jan 10, 2019

Spent address objects are continuing to persist correctly in the new db. Once the transactions are pruned, the address is injected into the spentAddresses db (Checked by searching the new db for addresses that were spent from following the pruning process). Attempts to double spend are rejected as expected, and the rejected transaction addresses are not persisted (no false positives found). While transactions are still present in the original db, validation catches attempts to spend from addresses that have already been spent from, and any chains of transactions that would lead to inconsistent ledger states are rejected, ensuring db integrity.


private RocksDB db;
// DBOptions is only used in initDB(). However, it is closeable - so we keep a reference for shutdown.
private DBOptions options;
private BloomFilter bloomFilter;
private boolean available;

public RocksDBPersistenceProvider(String dbPath, String logPath, int cacheSize) {
public RocksDBPersistenceProvider(String dbPath, String logPath, int cacheSize,

This comment has been minimized.

@@ -1,5 +1,6 @@
package com.iota.iri.controllers;

import com.iota.iri.Iota;

This comment has been minimized.

@@ -1,9 +1,11 @@
package com.iota.iri.storage.rocksDB;

import com.iota.iri.Iota;

This comment has been minimized.

@@ -4,6 +4,8 @@
import com.iota.iri.conf.TipSelConfig;
import com.iota.iri.controllers.TipsViewModel;
import com.iota.iri.controllers.TransactionViewModel;
import com.iota.iri.model.StateDiff;
import com.iota.iri.model.persistables.*;

This comment has been minimized.

@GalRogozinski GalRogozinski merged commit d43b949 into iotaledger:dev-localsnapshots Jan 10, 2019

5 checks passed

buildkite/iri-build-jar-prs Build #327 passed (10 minutes, 23 seconds)
Details
buildkite/iri-build-jar-prs/6131bc15-b64d-496d-911c-dd148a474b01 Passed (7 minutes, 59 seconds)
Details
buildkite/iri-build-jar-prs/build-iri-oracle8 Passed (1 minute, 50 seconds)
Details
buildkite/iri-build-jar-prs/pull-from-repo Passed (22 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment