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

Beacon chain block processing #1156

Merged
merged 21 commits into from
Aug 28, 2018
Merged

Conversation

mkalinin
Copy link
Contributor

@mkalinin mkalinin commented Aug 17, 2018

These changes are not ready for merge. PR is created in purpose of getting a feedback on general design of Beacon chain block processing part.

ToDo:

  • cover with tests
  • add javadocs

Resolves #1145

@mkalinin mkalinin added this to the Beacon milestone Aug 17, 2018
@mkalinin mkalinin requested a review from zilm13 August 17, 2018 03:58
@mkalinin mkalinin added this to On Review in Beacon chain via automation Aug 17, 2018
@coveralls
Copy link

coveralls commented Aug 17, 2018

Coverage Status

Coverage increased (+0.3%) to 55.981% when pulling 56a3739 on sharding/block-processing into ad9ab1c on research/sharding.

@mkalinin
Copy link
Contributor Author

PR is ready for merge now. Tests and docs have been added.

Copy link
Collaborator

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

Looks good!

try {
MessageDigest digest = MessageDigest.getInstance("BLAKE2B-512");
digest.update(data);
return Arrays.copyOf(digest.digest(), 32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

512 = 64 bytes, so you are getting only first half?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, exactly. That was a Vitalik's suggestion, cause not every lib supports configurable lengths for BLAKE2b. This suggestion was agreed among implementors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you, please, add comment about it so it will not confuse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return commonConfig.keyValueDataSource("beaconchain", settings);
}

private DbFlushManager beaconDbFlusher;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make it non-primary Bean instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Bean("beaconDbFlusher")
   public DbFlushManager beaconDbFlusher() {
         beaconDbFlusher = new DbFlushManager(systemProperties, Collections.emptySet(), beaconChainDbCache());
        shardingWorldManager.setBeaconDbFlusher(beaconDbFlusher);
        return beaconDbFlusher;
    }

and mark original dbflushmanager with @Primary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to keep it distant from ethereumj-core module and do not add changes in the code of it. Actually, beaconDbFlusher is not a fully functioning flush manager, it doesn't close databases for example, this is still lays on original flush manager. That is why it's created in such a dirty way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I got it!

}

public boolean isParentOf(Beacon other) {
return FastByteComparisons.equal(this.getHash(), other.getHash());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like equals, not parent of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍


public static final long SLOT = 0;

private static final byte[] NULL = new byte[32];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name looks weird!


/**
* Returns max block number that is presented in the store.
* @return max number or -1 is store is empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

if

* Saves block to the storage.
*
* @param block the block
* @param chainScore total score of the chain that is calculated after given block has been improted
Copy link
Collaborator

Choose a reason for hiding this comment

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

imported

byte[] shortList = RLP.encodeList(shortItem, shortItem, shortItem);
byte[] longList = RLP.encodeList(longItem, longItem, longItem);

byte[] encoded = RLP.wrapList(nullArray, singleZero, singleByte, shortItem, longItem, shortList, longList);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this
You have encodeElement for each data piece inside wrapList but prepared everything above with encodeElement too
I think you should put raw data instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not each element of wrapping list is encoded with encodeElement. Basically, this test checks that applying wrapList and then unwrapList results in data items that equal to original data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean that you test it on safe already encoded data, while it could be used with unsafe notencoded data.

@mkalinin mkalinin merged commit 3dcd158 into research/sharding Aug 28, 2018
Beacon chain automation moved this from On Review to Done Aug 28, 2018
@mkalinin mkalinin deleted the sharding/block-processing branch December 26, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Beacon chain
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants