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

Update policy - Add maximum witness size (in bytes) #979

Closed
lock9 opened this issue Aug 1, 2019 · 22 comments · Fixed by #1020
Closed

Update policy - Add maximum witness size (in bytes) #979

lock9 opened this issue Aug 1, 2019 · 22 comments · Fixed by #1020
Assignees
Labels
enhancement Type - Changes that may affect performance, usability or add new features to existing modules. network-policy Module - Issues that affect the network-policy like fees, access list and voting ready-to-implement Issue state: Ready to be implemented or implementation in progress

Comments

@lock9
Copy link
Contributor

lock9 commented Aug 1, 2019

This is needed for #840. It limits the size of the witness that can be used in the verification script.

@lock9 lock9 added this to the NEO 3.0 milestone Aug 1, 2019
@erikzhang
Copy link
Member

I think this can be a constant.

@shargon
Copy link
Member

shargon commented Aug 1, 2019

I think the same

@lock9
Copy link
Contributor Author

lock9 commented Aug 1, 2019

What would be the impact if we have to change this? Are you guys sure this won't change in the future? I think it would be a good practice to have this in the policy

@erikzhang
Copy link
Member

I don't think it will be modified often. This is a relatively stable limit value.

@shargon
Copy link
Member

shargon commented Aug 3, 2019

In fact, there is a limit now

InvocationScript = reader.ReadVarBytes(65536);
VerificationScript = reader.ReadVarBytes(65536);

We should reduce it to 512 or 1024?

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 3, 2019

I agree @shargon. Regarding value, we need to define max usage reasonable to offer for free.
in fact, its not free, right?
we are paying for size and computation as network fee now...

We are still discussing fee system I think, to avoid failed verifications to consume time from p2p edge.
we can put max size here, but without blocking loops, a max size is pointless.

We can follow "neo segwit" and do not store lon term witnesses, so max size is also pointless.

What is "reasonable" here? How many CN Neo3 expects to have validating blocks? Seven? So we max it to multisig 7/7 (to allow storing all sigs if wanted), in both invocation and verification. If CN increases, this increases automatically. This size will cover 99.9% of all existing applications, the rest can just be deployed.

@lock9
Copy link
Contributor Author

lock9 commented Aug 5, 2019

I understand that this value is not changed often, but what is the impact if we need this to change? Isn't this going to cause a "large mess"?

@lock9 lock9 added design Issue state - Feature accepted but the solution requires a design before being implemented enhancement Type - Changes that may affect performance, usability or add new features to existing modules. network-policy Module - Issues that affect the network-policy like fees, access list and voting labels Aug 9, 2019
@shargon shargon self-assigned this Aug 12, 2019
@shargon
Copy link
Member

shargon commented Aug 12, 2019

I think that 14/14 (or lower) won't need to be changed in the future, as igor said, the rest can be deployed

@lock9 lock9 added ready-to-implement Issue state: Ready to be implemented or implementation in progress and removed design Issue state - Feature accepted but the solution requires a design before being implemented labels Aug 13, 2019
@igormcoelho
Copy link
Contributor

@lock9 this won't affect future stuff, because it only checks during verification. Once it's on block, don't need to reverify (so we can shorten or increase this).

@shargon
Copy link
Member

shargon commented Aug 14, 2019

but is deserialized with the block, so it will affect if we decrease this value in the future

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 14, 2019

But we don't need to revalidate old tx, do we? Best thing is put this together with other network policies... so we definitely avoid this problem (it can increase/reduce, basis on storage).

@lock9
Copy link
Contributor Author

lock9 commented Aug 14, 2019

@igormcoelho so we should add this to the network policy? If we add this to the policy, we can see what was the value in a particular date and time, making it easier to change later.

@igormcoelho
Copy link
Contributor

I mean the PolicyContract (native one)... it will have the whole decisions recorded on chain itself, regarding network policy over time.

@shargon
Copy link
Member

shargon commented Aug 14, 2019

not in TX igor, but if you receive a block, you will deserialize it and you could obtain a FormatException if you decrease the value, if you increase it, there are no problem

@shargon
Copy link
Member

shargon commented Aug 14, 2019

But we have a lot of values with the same behavior, max entries for arrays (attributes, cosigners, etc..)

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 14, 2019

But really, the only reason to change this, is noticing huge spam attacks on border...

14/14 on invocation and verfication, allow for approximately:
invocation => 14 x 64 bytes (signatures) = 896 bytes
verification => 14 x 33 bytes (pubkeys) = 462 bytes
plus small stuff, we have around 1500 bytes maximum per witness (which is A LOT in my opinion...)

If some devil soul tries to use expensive stuff, and fault, this is an opportunity for:
1500 / 5 bytes (each syscall in compressed format) = 300 checksig

each checksig takes few microseconds, but 300 may take 0,1ms or more (we need to calculate this). If this "good" soul repeats several tx like this, with difference nonce, it may take considerable amount of time from network edge... with only solution by blacklisting the ip (in my opinion).

If we also count-limit the expensive stuff on verification, let's say, by 14 operations... multisig 14/14 still works, and damage is pretty lowered. Someone may try to vary other expensive stuff, like hashing, etc, but if all of these remain inside same "quota", it will be pretty hard to attack with useless verifications.

@shargon
Copy link
Member

shargon commented Aug 14, 2019

Yes, it's a lot of

@shargon
Copy link
Member

shargon commented Aug 14, 2019

We can decrease it to 10/10, it will be safe if we want to increase it in the future

@igormcoelho
Copy link
Contributor

10/10 may give us around 1000 bytes, I think so. Still 200 checksig, but much better. Expensive op counting may help too, but this is only important on borders. "Inside network", we can be very strict, if someone routes bad tx via p2p it should be blacklisted immediately (at least temporarily).

@shargon
Copy link
Member

shargon commented Aug 14, 2019

@igormcoelho feel free to change my PR with your desired values

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 14, 2019

is there a PR? Found it: #1020

@igormcoelho
Copy link
Contributor

@shargon we should definitely adopt a Neo SegWit design now: #1029

We only need to deny access for VerificationScript on interops.. (invocation is already denied on neo2 due to non-deterministic/maleable nature). So, after a few thousand blocks, we can provide shorter blocks and tx, completely without witnesses. In this sense, this size here is just an instantaneous protection against p2p network edge.

@erikzhang erikzhang removed this from the NEO 3.0 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Changes that may affect performance, usability or add new features to existing modules. network-policy Module - Issues that affect the network-policy like fees, access list and voting ready-to-implement Issue state: Ready to be implemented or implementation in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants