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

GetBlocks by block index #1397

Open
wants to merge 69 commits into
base: master
from
Open

Conversation

@ShawnYun
Copy link
Contributor

ShawnYun commented Jan 3, 2020

Closes #1378.
As the discussion at #1378 ,we will modify the synchronization mechanism in two steps:

Use index directly for synchronization;
Add the assessment of node health.
This PR is the draft of the first step, which is mainly modified in the following aspects:

Deleted the original logic of synchronous block header;
Modified the processing of receiving the new block in Blockchain;
SyncManager has been added to manage synchronization tasks.
If you have any comments, please let me know

ShawnYun added 22 commits Nov 26, 2019
merge
update
merge
Neo.VM v3.0.0-CI00174 (#1320)
merge
merge
update
fix
Neo.VM.3.0.0-CI00201 (#1374)
…o get-full-blocks
@ShawnYun ShawnYun marked this pull request as ready for review Jan 8, 2020
@Tommo-L

This comment has been minimized.

Copy link
Contributor

Tommo-L commented Jan 8, 2020

@erikzhang @ixje @vncoelho @shargon Could help review it, plz

@ixje

This comment has been minimized.

Copy link
Contributor

ixje commented Jan 8, 2020

I had a quick look and the first thing I noticed is that there is a new MessageCommand.BlockData. Why do we need this instead of using the existing Block?

Both messages deliver the same payload (of type Block)

ShawnYun added 2 commits Feb 17, 2020
merge
@ShawnYun ShawnYun changed the title Add Get-full-blocks GetBlocks by block index Feb 18, 2020
@shargon shargon added the blocker label Feb 18, 2020
@Tommo-L Tommo-L mentioned this pull request Feb 20, 2020
5 of 7 tasks complete
src/neo/Ledger/Blockchain.cs Outdated Show resolved Hide resolved
erikzhang added 2 commits Feb 21, 2020
@cloud8little

This comment has been minimized.

Copy link
Contributor

cloud8little commented Feb 21, 2020

Smoke Test Passed for single node CN.
Speed Test for Syncing block comparison:

offline package sync:
No obevious difference, but with this pr, the offline syncing is a bit slower.

block height baseline pr1397
10057 25.51 27.95
20043 49.59 54.64

P2P Sync time(seconds) with StatesDumper intalled (start from index 0)
Improve about 7 times.

block height baseline pr1397
21130 (start from 0) 750 98.66

P2P Sync time(seconds) without StatesDumper intalled (start from index 30)
Improve about 4~5times.

block height baseline pr1397
21130 450 95.01
21149 510 82

will do some functional explore test later and update the result.

【Update】Deploy nep5 contract successfully, four node consensus passed.

ShawnYun added 2 commits Feb 21, 2020
Modify get headers
src/neo/Ledger/Blockchain.cs Outdated Show resolved Hide resolved
@Tommo-L Tommo-L mentioned this pull request Mar 10, 2020
@Tommo-L

This comment has been minimized.

Copy link
Contributor

Tommo-L commented Mar 13, 2020

Could we review this PR.

We want to add node health and security mechanism, and move transaction verification to RemoteNode based on this PR.

shargon and others added 3 commits Mar 13, 2020
erikzhang and others added 3 commits Mar 26, 2020
…o get-full-blocks

namespace Neo.Network.P2P.Payloads
{
public class GetHeadersPayload : ISerializable

This comment has been minimized.

Copy link
@erikzhang

erikzhang Mar 27, 2020

Member

I think this is the same as GetBlockDataPayload. Maybe we can use only one payload. It can be renamed to GetBlockByIndexPayload.

This comment has been minimized.

Copy link
@ShawnYun

ShawnYun Mar 27, 2020

Author Contributor

Yes. Renaming it will make it easier to understand.

This comment has been minimized.

Copy link
@ShawnYun

ShawnYun Mar 30, 2020

Author Contributor

Done

vncoelho and others added 3 commits Mar 27, 2020
…o get-full-blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.