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.

Sign up for GitHub

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

Jump to bottom

Refactor blocks endpoint - Closes #4369 #4372

Merged
merged 4 commits into from Oct 10, 2019
Merged

Refactor blocks endpoint - Closes #4369 #4372

merged 4 commits into from Oct 10, 2019

Conversation

2snEM6
Copy link
Contributor

@2snEM6 2snEM6 commented Oct 7, 2019

What was the problem?

Refer to #4369

How did I solve it?

  • Renaming action to getBlocksFromID

How to manually test it?

To do

Review checklist

@2snEM6 2snEM6 self-assigned this Oct 7, 2019
2snEM6
2snEM6 commented Oct 7, 2019
View reviewed changes
framework/src/modules/chain/transport/transport.js Outdated
// Get 34 blocks with all data (joins) from provided block id
// According to maximum payload of 58150 bytes per block with every transaction being a vote
// Discounting maximum compression setting used in middleware
// Maximum transport payload = 2000000 bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shuse2 @ishantiw
I am tagging you here to catch up on what is the current maximum payload size set for our P2P layer as this will directly affect the number of blocks we can return.

Copy link
Member

Choose a reason for hiding this comment

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

Right now by default max payload size is 3048576 bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishantiw can you elaborate on why is that? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Initially, we had maxPayloadLimit around 2000000 bytes but when we were syncing with this limit, there were some packets having size higher than this limit and it was not syncing after that as it was rejected. To support syncing until the current height we decided to increase it to keep it 3048576 bytes~3mb.

Copy link
Contributor Author

@2snEM6 2snEM6 Oct 7, 2019 •
edited

Choose a reason for hiding this comment

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

Thanks @ishantiw. @shuse2 does this statement // According to maximum payload of 58150 bytes per block with every transaction being a vote still apply?

Copy link
Member

Choose a reason for hiding this comment

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

I think since we change the syncing format on this branch, it should be better.
We need to check size of a block with full vote with 25 transactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After taking with @shuse2 let's leave it to 34 blocks and discuss this in another spike issue

shuse2
shuse2 requested changes Oct 8, 2019
View reviewed changes
framework/src/modules/chain/schema/definitions.js Outdated Show resolved Hide resolved
framework/src/modules/chain/transport/transport.js Outdated
// Get 34 blocks with all data (joins) from provided block id
// According to maximum payload of 58150 bytes per block with every transaction being a vote
// Discounting maximum compression setting used in middleware
// Maximum transport payload = 2000000 bytes
Copy link
Member

Choose a reason for hiding this comment

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

I think since we change the syncing format on this branch, it should be better.
We need to check size of a block with full vote with 25 transactions.

framework/src/modules/chain/transport/transport.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 force-pushed the 4369_refactor_blocks_endpoint branch 3 times, most recently from 3600285 to b916a15 Compare October 8, 2019 08:44
@2snEM6 2snEM6 marked this pull request as ready for review October 8, 2019 08:47
@2snEM6 2snEM6 requested review from shuse2 and ishantiw October 8, 2019 08:47
ishantiw
ishantiw reviewed Oct 8, 2019
View reviewed changes
Copy link
Member

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor comments related to naming and comments

framework/src/modules/chain/chain.js Outdated Show resolved Hide resolved
framework/src/modules/chain/transport/transport.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 force-pushed the 4369_refactor_blocks_endpoint branch from b916a15 to 0e6390e Compare October 8, 2019 13:01
@2snEM6 2snEM6 requested a review from ishantiw October 8, 2019 13:59
@2snEM6 2snEM6 force-pushed the 4369_refactor_blocks_endpoint branch from c672641 to f8a157d Compare October 9, 2019 12:21
@2snEM6 2snEM6 force-pushed the 4369_refactor_blocks_endpoint branch from f8a157d to bc7cb16 Compare October 9, 2019 13:24
@shuse2 shuse2 merged commit f1a3cfe into feature/introduce_bft_consensus Oct 10, 2019
@shuse2 shuse2 deleted the 4369_refactor_blocks_endpoint branch October 10, 2019 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shuse2 shuse2 shuse2 approved these changes

@ishantiw ishantiw ishantiw approved these changes

Assignees

@2snEM6 2snEM6

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@2snEM6 @shuse2 @ishantiw