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

Optimize consensus messages #1971

Closed
wants to merge 4 commits into from
Closed

Optimize consensus messages #1971

wants to merge 4 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Sep 25, 2020

Related to #1963

We can save 2 bytes more if we send only the InvocationScript instead of the Witness

@erikzhang
Copy link
Member

I think it is not worth optimizing. Because ConsensusPayload will not be stored.

@shargon
Copy link
Member Author

shargon commented Sep 25, 2020

But the messages are smaller

@roman-khimov
Copy link
Contributor

I think we can even go as far as

if (Witness.VerificationScript.Length != 0) {
    return false
}

Or really just leave the invocation script in the payload. And then remove GetNextBlockValidators()/CreateSignatureRedeemScript() from GetScriptHashesForVerifying() as it's already done in the Verify.

Smaller messages are always good to me, especially if they cost nothing like here. We need ConsensusPayloads to be signed by very specific keys, we know them all, so carrying these scripts with every payload just to check that they really are the same as the ones we produce in GetScriptHashesForVerifying() is just a waste of bandwidth (and time to serialize/deserialize them, and memory to hold them for a while). Not a huge waste, but still lowering it to zero can't be bad.

@shargon
Copy link
Member Author

shargon commented Jan 29, 2021

It should be done in neo-modules, I think that it's a good solution to speed up p2p protocol, if we decide to use it, I can copy the code to neo-modules.

@shargon shargon closed this Jan 29, 2021
@shargon shargon deleted the optimize-consensus-msg branch January 29, 2021 09:25
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