Skip to content

Feature/wire#133

Merged
mkalinin merged 100 commits intodevelopfrom
feature/wire
May 27, 2019
Merged

Feature/wire#133
mkalinin merged 100 commits intodevelopfrom
feature/wire

Conversation

@Nashatyrev
Copy link
Copy Markdown
Contributor

Implement wire protocol, add regular sync, peers and connections management

Copy link
Copy Markdown
Member

@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 perfect! Just some thoughts when looking around.

ExpiredBlock,
InvalidBlock,
StateMismatch,
OtherError
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd throw out OtherError until it is needed. Maybe we could keep staying with more concrete results.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, UnexpectedError reads better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And I think we definitely need a StateTransitionError to reflect SpecAssertionException that is thrown by the spec.


BeaconBlockStorage getBlockStorage();

DataSource<Hash32, BeaconBlockHeader> getBlockHeaderStorage();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not wrap it with dedicated interface like others. Or, lets postpone it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's postpone, since I'm not yet sure how this header storage should be organized

}
}

void chainStarted(ChainStart chainStartEvent) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we split this method to few smaller readable tasks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, good question. I wanted to return to this when create it

public abstract class ValidatorKeys {

public static class Generate extends ValidatorKeys {
private int count;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be great to see something like 10 per epoch or, say, one per every 10 slots.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what do you mean...

timeParameters:
SECONDS_PER_SLOT: 10
MIN_ATTESTATION_INCLUSION_DELAY: 1
SLOTS_PER_EPOCH: 2 No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new line, please!

* this block + all its descendants returned
*
* Blocks with height less than {@link #getTopBlock()} are dropped
* Blocks with height bigger than some threshold above {@link #getTopBlock()} are dropped
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

some threshold above sounds really like magic

Copy link
Copy Markdown
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

Network stack and sync process based on RX streams is something outstanding! Happy to merge this one.

@mkalinin mkalinin merged commit 1451a47 into develop May 27, 2019
@mkalinin mkalinin deleted the feature/wire branch July 16, 2019 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants