[NC-1344] Create a simple WorldStateDownloader #657

mbaxter · 2019-01-24T23:45:40Z

PR description

This PR contains a basic algorithm for downloading world state from the network along with supporting changes. This is just the first step in implementing the WorldStateDownloader. Additional functionality will need to be implemented in subsequent PRs including:

A real queue implementation that handles serialization / deserialization of data persisted to disk
Cancellation functionality
Handling stalled downloads
Periodic commits of downloaded world state

Add some small optimizations related to persisting empty code.

ajsutton

This is looking really promising. I suspect most of the things I've noted could be pushed to follow up PRs if you prefer. The only thing really worrying me is exposing StoredNode and using instanceof with it.

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AccountTuple.java

...m/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/WorldStateStorage.java

ajsutton · 2019-01-25T00:15:53Z

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/EthServer.java

+        nodeData.add(BytesValue.EMPTY);
+      } else {
+        worldStateArchive.getNodeData(hash).ifPresent(nodeData::add);
+      }


This check would probably be better inside WorldStateArchive so any future callers automatically benefit. Sorry, I should have done that in the first place...

Yeah, this makes more sense in WorldStateArchive. I also added the optimization to the KeyValueStorageWorldStateStorage. Added some tests and cleaned up a few other things along the way, so probably worth looking through this commit: 03018c5

...h/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/worldstate/WorldStateDownloader.java

ajsutton · 2019-01-25T00:54:25Z

...h/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/worldstate/WorldStateDownloader.java

+                      future.complete(null);
+                    } else {
+                      // Send out additional requests
+                      requestNodeData();


This looks like we're sending a single request for data at a time, processing it and then sending another one. I think we probably should be sending multiple requests at a time to speed up the download (with requests spread across multiple peers). Not required for this first PR but probably something to follow up on.

requestNodeData has a while loop, so it should send out as many requests as it can whenever it runs

So it does. I wonder if it's worth extracting a method for the send single request function to make that while a bit easier to spot (though sometimes there's just no helping fools like me...).

Line 128 above:
if (outstandingRequests.decrementAndGet() == 0 && pendingRequests.isEmpty()) {
will then depend on isEmpty() being thread safe and completely accurate. For the in memory implementation it would be enough to delegate to ConccurrentLinkedQueue.isEmpty() rather than using the separately tracked size variable I think. We know we're the last request to finish so there's nothing else coming in at the same time, but we need to be sure that any pending requests added by the request prior to us have actually been reflected in the pendingRequests properly.

ajsutton · 2019-01-25T00:58:29Z

...h/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/worldstate/WorldStateDownloader.java

+                  (res, error) -> {
+                    if (outstandingRequests.decrementAndGet() == 0 && pendingRequests.isEmpty()) {
+                      // We're done
+                      worldStateStorageUpdater.commit();


Something to keep an eye on - if we want to be able to resume download after being interrupted we'll need to commit to storage more often. Otherwise everything would be lost if Pantheon was terminated while still downloading. Also worth testing how long RocksDB takes to commit if you happen to do it for the entire MainNet world state in one go...

yep - i've got a subtask for that

ajsutton · 2019-01-25T01:04:51Z

ethereum/trie/src/main/java/tech/pegasys/pantheon/ethereum/trie/StoredNode.java

 import java.util.Optional;

-class StoredNode<V> implements Node<V> {
+public class StoredNode<V> implements Node<V> {


This feels like an implementation detail that just leaked and really shouldn't have. I can see we wind up doing an instanceof in TrieNodeDataRequest but it's unclear exactly why it's specifically checking for this class or why isLoaded is a meaningful method to call. Maybe Node needs a new method specifically to address this situation like isReferencedByHash?

Yeah, meant to clean this up. Added the method you suggested which seems to be the thing we really need.

ajsutton · 2019-01-25T01:07:39Z

ethereum/trie/src/main/java/tech/pegasys/pantheon/ethereum/trie/StoredNodeFactory.java

+  public static StoredNodeFactory<BytesValue> create() {
+    return new StoredNodeFactory<>(
+        (h) -> Optional.empty(), Function.identity(), Function.identity());
+  }


This seems fairly misleading - it doesn't really create a very useful StoredNodeFactory. Looks like we really just wanted to split out the decoding functionality to be separate from the retrieve and create functionality.

Yeah - this is kind of messy. Created a TrieNodeDecoder helper class instead.

ajsutton · 2019-01-25T01:16:57Z

services/queue/src/main/java/tech/pegasys/pantheon/services/queue/InMemoryBigQueue.java

+
+public class InMemoryBigQueue<T> implements BigQueue<T> {
+  private final AtomicLong size = new AtomicLong(0);
+  private final Queue<T> internalQueue = new ConcurrentLinkedQueue<>();


Tracking these separately means this implementation is not really thread-safe (size and actual requests may be out of sync). size and isEmpty are always somewhat dangerous methods for a queue used concurrently because the state may change before you actually try to act on the result.

I think it's ok at the moment, but if we move to having multiple outstanding requests then we probably need to rethink how we use this and outstandingRequests back in WorldStateDownloader.

Removed the separate size tracking. I think that the way isEmpty is being used in the downloader should be okay because the flow is:

send request and increment outstandingRequest count

receive response and process it

queue additional requests

decrement outstandingRequests and check if the queue is empty

Assuming that once requests are queued (enqueue returns), an updated count should be visible to other threads, I think that flow works. Does that seem right to you?

As long as the outstanding request count is incremented before the request is actually sent (which it was from memory) then yes I agree that works.

Clean up AccountState handling of account value

Fully implement this optimization for world state archive and storage. Make world storage api consistent.

We should requeue requests when an error is encountered.

mbaxter · 2019-01-25T21:33:44Z

...n/java/tech/pegasys/pantheon/ethereum/storage/keyvalue/KeyValueStorageWorldStateStorage.java

@@ -29,23 +32,46 @@ public KeyValueStorageWorldStateStorage(final KeyValueStorage keyValueStorage) {
  }

  @Override
-  public Optional<BytesValue> getCode(final Hash codeHash) {
-    return keyValueStorage.get(codeHash);
+  public Optional<BytesValue> getCode(final Bytes32 codeHash) {


Some of these methods use Hash and some use Bytes32 - just changed them all to Bytes32 since that is more generic.

It's also kind of annoying to have to wrap Bytes32 in Hash all over. Kind of wondering if we really need the Hash type ...

I like Hash providing a very clear type to say this is a hash, not just an arbitrary 32 bytes but I don't think it has to be a hard and fast "every hash must use the Hash type" kind of thing. Particularly in this low level kind of place where the meaning is unambiguous using Bytes32 makes sense to me.

mbaxter · 2019-01-25T21:37:04Z

...m/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java

@@ -49,7 +49,7 @@
  private final WorldStateStorage worldStateStorage;

  public DefaultMutableWorldState(final WorldStateStorage storage) {
-    this(MerklePatriciaTrie.EMPTY_TRIE_ROOT_HASH, storage);
+    this(MerklePatriciaTrie.EMPTY_TRIE_NODE_HASH, storage);


There's nothing special about a root node, so updated this constant name so that it can be used more generally.

ajsutton

LGTM.

ajsutton · 2019-01-25T21:42:46Z

...n/java/tech/pegasys/pantheon/ethereum/storage/keyvalue/KeyValueStorageWorldStateStorage.java

+
+  private Optional<BytesValue> getValue(
+      final Bytes32 hash, final Function<Bytes32, Optional<BytesValue>> getter) {
+    return getTrieValue(hash, (h) -> getCodeValue(h, getter));


I think we've wound up applying the check for empty twice in the getNodeData path - once in WorldStateArchive and once here. This is probably the better place, but I actually prefer the more explicit code from WorldStateArchive - I know it essentially duplicates some code but explicitly checking for EMPTY and EMPTY_TRIE_NODE_HASH is just a bit more readable than delegating through optionals.

I'm not sure it matters, but it also avoids creating an extra object instances that this lambda version requires (to create a closure over the getter param).

Fair enough - updated

ajsutton · 2019-01-25T21:46:27Z

...n/java/tech/pegasys/pantheon/ethereum/storage/keyvalue/KeyValueStorageWorldStateStorage.java

@@ -29,23 +32,46 @@ public KeyValueStorageWorldStateStorage(final KeyValueStorage keyValueStorage) {
  }

  @Override
-  public Optional<BytesValue> getCode(final Hash codeHash) {
-    return keyValueStorage.get(codeHash);
+  public Optional<BytesValue> getCode(final Bytes32 codeHash) {


I like Hash providing a very clear type to say this is a hash, not just an arbitrary 32 bytes but I don't think it has to be a hard and fast "every hash must use the Hash type" kind of thing. Particularly in this low level kind of place where the meaning is unambiguous using Bytes32 makes sense to me.

ajsutton · 2019-01-25T21:53:22Z

...h/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/worldstate/WorldStateDownloader.java

+                      future.complete(null);
+                    } else {
+                      // Send out additional requests
+                      requestNodeData();


So it does. I wonder if it's worth extracting a method for the send single request function to make that while a bit easier to spot (though sometimes there's just no helping fools like me...).

Line 128 above:
if (outstandingRequests.decrementAndGet() == 0 && pendingRequests.isEmpty()) {
will then depend on isEmpty() being thread safe and completely accurate. For the in memory implementation it would be enough to delegate to ConccurrentLinkedQueue.isEmpty() rather than using the separately tracked size variable I think. We know we're the last request to finish so there's nothing else coming in at the same time, but we need to be sure that any pending requests added by the request prior to us have actually been reflected in the pendingRequests properly.

mbaxter added 12 commits January 23, 2019 12:00

In progress - start writing WorldStateDownloader

9593d1f

Queue additonal requests as node data comes back from the network

fad8ce1

Fix bug

e1178b3

Fix some warnings, start adding tests

1b2dff6

Finish first downloader test, fix some issues related codeless accounts

17114a8

Add some small optimizations related to persisting empty code.

Add final's

ac572f3

Rename some classes and variable for clarity

2ef2c6a

Rework downloader w thread-safety in mind

e205567

Clarify some method names

91d7f6d

Fix comment

4928b41

Fix bug - add missing break statement

3af28bd

Add more tests

98d808d

mbaxter requested a review from ajsutton January 24, 2019 23:45

Minor cleanup

cce5e7e

ajsutton reviewed Jan 25, 2019

View reviewed changes

ajsutton mentioned this pull request Jan 25, 2019

[NC-2137] Start world downloader #658

Merged

mbaxter added 8 commits January 25, 2019 11:11

Rename AccountTuple and move it into the worldstate package

365c32d

Clean up AccountState handling of account value

Remove TODO comments

6e1a67c

Skip storage lookup for empty code and empty trie nodes

03018c5

Fully implement this optimization for world state archive and storage. Make world storage api consistent.

Fix some issues related to thread-safety, fix error-handling bug

ac98c31

We should requeue requests when an error is encountered.

Cut obsolete test

af5ff05

Clean up trie node handling

c57737a

Create a TrieNodeDecoder helper

aa0394b

Merge branch 'master' into nc-1344/download-world-state

c996fdf

mbaxter commented Jan 25, 2019

View reviewed changes

Fix method names, remove unintended changes

6ee99e7

ajsutton approved these changes Jan 25, 2019

View reviewed changes

mbaxter added 2 commits January 25, 2019 19:57

Merge branch 'master' into nc-1344/download-world-state

e111aa7

Make KeyValueWorldStateStorage more readable, cut duplicate checks

3cc3b9c

Fix javadoc

30858a2

mbaxter changed the title ~~Nc 1344/download world state~~ [NC-344] Create a simple WorldStateDownloader Jan 26, 2019

mbaxter changed the title ~~[NC-344] Create a simple WorldStateDownloader~~ [NC-1344] Create a simple WorldStateDownloader Jan 26, 2019

mbaxter merged commit dff436b into PegaSysEng:master Jan 26, 2019

rain-on pushed a commit to rain-on/pantheon that referenced this pull request Jan 29, 2019

[NC-1344] Create a simple WorldStateDownloader (PegaSysEng#657)

66ce6ee

vinistevam pushed a commit to vinistevam/pantheon that referenced this pull request Jan 29, 2019

[NC-1344] Create a simple WorldStateDownloader (PegaSysEng#657)

8979a48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NC-1344] Create a simple WorldStateDownloader #657

[NC-1344] Create a simple WorldStateDownloader #657

mbaxter commented Jan 24, 2019 •

edited

Loading

ajsutton left a comment

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton Jan 25, 2019

mbaxter Jan 25, 2019

ajsutton left a comment

ajsutton Jan 25, 2019

mbaxter Jan 26, 2019

ajsutton Jan 25, 2019

ajsutton Jan 25, 2019

[NC-1344] Create a simple WorldStateDownloader #657

[NC-1344] Create a simple WorldStateDownloader #657

Conversation

mbaxter commented Jan 24, 2019 • edited Loading

PR description

ajsutton 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

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

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

ajsutton 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

mbaxter commented Jan 24, 2019 •

edited

Loading