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

confirm_req_hash to reduce bandwidth usage #1046

Merged
merged 74 commits into from Jan 28, 2019

Conversation

Projects
6 participants
@SergiySW
Copy link
Collaborator

commented Aug 10, 2018

Initially support for representative side.
Once network is upgraded, replacing send_confirm_req with send_confirm_req_hash in next version

@SergiySW SergiySW requested a review from PlasmaPower Aug 20, 2018

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Why include the block hash in the message? Just the root should do.

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2018

Actually I thought about answering with full block if fork & just hash vote if same block requested

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2018

Except that hash can be removed from request (& renamed to confirm_req_root)

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

If root is zero, we should just check if we have the block. That way, this can still be used to confirm frontiers before pulling them.

We should probably also support batching here.

@Inkeliz

This comment has been minimized.

Copy link

commented Aug 29, 2018

It's very strange. We have one single confirm_ack that supports votes by hash and vote by block, simply using the Extensions, in the header. However, the confirm_req got a totally new type, the confirm_req_hash.

Why not follow the same way of confirm_ack, usingExtensions header. 👎

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2018

It should be possible as well

@rkeene rkeene added this to the V17.0 milestone Aug 29, 2018

@zhyatt zhyatt requested review from wezrule and cryptocode Jan 22, 2019

@cryptocode
Copy link
Collaborator

left a comment

LGTM pending the loop optimization comment

Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/core_test/message_parser.cpp Outdated
Show resolved Hide resolved nano/core_test/block.cpp Outdated
Show resolved Hide resolved nano/core_test/message_parser.cpp Outdated
Show resolved Hide resolved nano/node/common.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated
Show resolved Hide resolved nano/node/node.cpp Outdated

@SergiySW SergiySW merged commit 3d81f16 into nanocurrency:master Jan 28, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.