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

Set max block size #953

Merged
merged 54 commits into from Aug 20, 2019

Conversation

@shargon
Copy link
Member

commented Jul 24, 2019

Close #359

Default value 256 Kb per block

@shargon shargon requested review from erikzhang, vncoelho, lock9 and igormcoelho Jul 24, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 24, 2019

Codecov Report

Merging #953 into master will increase coverage by 0.51%.
The diff coverage is 91.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #953      +/-   ##
==========================================
+ Coverage   60.83%   61.35%   +0.51%     
==========================================
  Files         195      195              
  Lines       13332    13405      +73     
==========================================
+ Hits         8110     8224     +114     
+ Misses       5222     5181      -41
Impacted Files Coverage Δ
neo/Network/P2P/Payloads/Block.cs 89.77% <ø> (ø) ⬆️
neo/Consensus/ConsensusService.cs 15.53% <0%> (-0.18%) ⬇️
neo/SmartContract/Native/PolicyContract.cs 94.23% <100%> (+1.2%) ⬆️
neo/Network/P2P/Payloads/BlockBase.cs 82.14% <100%> (+7.14%) ⬆️
neo/Consensus/ConsensusContext.cs 67.39% <96.42%> (+12.61%) ⬆️
neo/Network/P2P/Payloads/ConsensusPayload.cs 80.55% <0%> (+4.16%) ⬆️
neo/Persistence/Helper.cs 8.33% <0%> (+8.33%) ⬆️
neo/Ledger/TrimmedBlock.cs 21.25% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a705b43...f42a7c9. Read the comment docs.

@vncoelho vncoelho added this to the NEO 3.0 milestone Jul 24, 2019

@vncoelho
Copy link
Member

left a comment

Perfect, @shargon.

We still need that before sending PrepareResponse, OnPrepReqReceived, nodes also check that.

neo/Consensus/ConsensusContext.cs Outdated Show resolved Hide resolved

@shargon shargon marked this pull request as ready for review Jul 25, 2019

shargon added 3 commits Jul 25, 2019

@shargon shargon requested a review from vncoelho Jul 25, 2019

@igormcoelho
Copy link
Contributor

left a comment

I think the idea is quite good... it doesn't affect economic model, while protects network from unexpected situations.
Regarding implementation: isn't it possible to embed all these verifications during Block verification? If that was possible, we would need to change only few extra lines of code... but don't know if it's feasible in fact, just guessing.

@lock9
Copy link
Contributor

left a comment

How do you test this?

neo/Consensus/ConsensusContext.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
shargon added 2 commits Jul 26, 2019

@shargon shargon requested a review from erikzhang Jul 26, 2019

@vncoelho
Copy link
Member

left a comment

Nice very good, @shargon.

neo/Consensus/ChangeViewReason.cs Show resolved Hide resolved
neo/Consensus/ConsensusContext.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusContext.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusContext.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
@vncoelho

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Last committed destroyed indentation...
@shargon, can you revert to me? I tried to push in the branch but I have no write on your repository to push via ssh.
I do not understand how this works, because from github I could push.

I edited online and it make all these changes on the indentation, quite strange.

vncoelho and others added 5 commits Jul 30, 2019
@vncoelho

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Editing online is destroying everything " We’ve detected the file has mixed line endings. When you commit changes we will normalize them to Windows-style (CRLF)."

vncoelho added 2 commits Aug 12, 2019
Revert "Remove extra line"
This reverts commit e134881.

@shargon shargon requested a review from erikzhang Aug 12, 2019

@igormcoelho
Copy link
Contributor

left a comment

Will we allow more than M signatures in a valid block? I think so...

@shargon

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

We should write the blocks only with M signatures, but i will review it

@shargon

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@igormcoelho As i thought we only attach to the block the minimum required && j < M

for (int i = 0, j = 0; i < Validators.Length && j < M; i++)

@igormcoelho

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@igormcoelho As i thought we only attach to the block the minimum required && j < M

for (int i = 0, j = 0; i < Validators.Length && j < M; i++)

Shargon, I know that CN algorithm currently does this (limited to M), but it's strange to not be able to even store other commits, I mean, on P2P level, the more information you have, the better. Blockchain explorers could also collect more signatures to publicly display which nodes signed the blocks, perhaps all 7? This change makes this impossible.
Perhaps we should link this block limit into two parts: (1) block header plus tx limit (2) witness limit. So, we directly take witness limit from that other PR (we need to expose public variables), and use it here.

If we don't do it, the other witness size proposal may be inconsistent... you may have a valid witness, but block itself won't accept it. You put more nodes as CN, fits on witness, but needs to reduce tx due to block limit.

Think about it, some node relays more signatures than you do, script is correct, witness is correct, but you.will deny by other reason, strange. Witnesses are disposable in time, even block witnesses, if you.have next block signed, previous is fine. We need to block tx grouped limit, not single witness limit.

@igormcoelho
Copy link
Contributor

left a comment

I think we should simply adopt Witness.MaxSize and avoid all this extra internal calculation, that may lead to complications.

{
sb.EmitPush(new byte[64]);
}
_witnessSize = new Witness

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 15, 2019

Contributor

This is specially bad if we dont drop it, after cleaning next context, and eventually.number of nodes increases (but we keep same previous _witnessSize, and block may break). It's better if we attain to a simple const: Witness.GetMaxSize() or Witness.MaxSize (or even better, we just limit BlockHeader size + tx)

@shargon

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

#953 (comment)

@igormcoelho If p2p nodes collect more signatures than M is because they have a modified code of neo, so they can modify the deserialization part too.

@shargon

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

Your proposal is remove the real witness size and replace it with the maximum witnesses size? what is the benefits of that? the calculation of this size is already done

@igormcoelho
Copy link
Contributor

left a comment

I prefer with.more than M sigs, but it can work like this.

Your proposal is remove the real witness size and replace it with the maximum witnesses size? what is the benefits of that? the calculation of this size is already done

Calculation is done, but it's unnecessary piece of code to maintain, if we simply get this info from another class. If we guarantee a full N/N multisig for nodes, others will be able to store complete information (in valid block), even if "standard" C# neo nodes doesn't do this. I find this beneficial for many reasons, it's much stronger guarantee that blocks are correct in my opinion, and useful for voting (more nodes participating on blocks, the better, if never participating, something is bad with node).

igormcoelho and others added 3 commits Aug 16, 2019

@shargon shargon requested review from lock9 and removed request for lock9 Aug 20, 2019

@lock9
lock9 approved these changes Aug 20, 2019
@shargon

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@igormcoelho We can discuss in a new issue if we need to change M to more than M, but we should change the consensus logic too (with configuration or something like that)

StoreWitnesses=[min,max]

@shargon shargon merged commit e792898 into neo-project:master Aug 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shargon shargon deleted the shargon:max-block-size branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.