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

[Neo3] Witnesses should deny access to Verification and Invocation script onchain #1029

Closed
igormcoelho opened this issue Aug 15, 2019 · 8 comments · Fixed by #1081
Closed
Labels
enhancement Type - Changes that may affect performance, usability or add new features to existing modules. ready-to-implement Issue state: Ready to be implemented or implementation in progress vm New features that affect the Neo Virtual Machine or the Interop layer

Comments

@igormcoelho
Copy link
Contributor

igormcoelho commented Aug 15, 2019

I think we shouldn't give access to Verification or Invocation script on Neo3.
InvocationScript is already non-deterministic, and cut-off on Neo2, but Verification can also be not good to access, if we follow a "Neo SegWit" logic... If we use it on script, we need it to execute the script, so this ties us forever to maintaining that witness on blockchain.
If user really wants to handle VerificationScript, it should pass it manually on Entry (Script) to the contract, that takes the Hash160 and matches against a known witness.
It's much better for network optimizations (we can easily drop all Witnesses in short time periods).

@igormcoelho
Copy link
Contributor Author

Related to: #906 (this allows us to safely name a onchain feature as "Witness", and it would be corresponding to a Verification ScriptHash)

Related to: #840 (Witness price is just instantaneous price, not future prices).

@igormcoelho igormcoelho added discussion Initial issue state - proposed but not yet accepted easy-to-do ready-to-implement Issue state: Ready to be implemented or implementation in progress vm New features that affect the Neo Virtual Machine or the Interop layer labels Aug 16, 2019
@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 16, 2019

Ok, we got the blessing of master yoda here... so I'll put it ready-to-implement xD

This is an easy task, good to invite new souls to interact :)

Here's the specification:

  • Remove Neo.Witness.GetVerificationScript interop call
  • Remove Neo.Transaction.GetWitnesses interop call (since Neo.Witness.* prefix will cease to exist).

After this is done, some stuff can be done on neo-devpack-dotnet too... basically remove Witness class and GetWitness call from Transaction.

@lock9
Copy link
Contributor

lock9 commented Aug 16, 2019

Easy to do? 😂
Please, let's leave at least 2 days in the design phase so are sure this is implemented correctly.
Thanks

@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. and removed discussion Initial issue state - proposed but not yet accepted easy-to-do ready-to-implement Issue state: Ready to be implemented or implementation in progress labels Aug 16, 2019
@shargon
Copy link
Member

shargon commented Aug 16, 2019

Pros and cons of this proposal?

@igormcoelho
Copy link
Contributor Author

I only see pros.
Cons would exist on neo2, as a breaking change
On neo3 thats fine, people can still implement same stuff by explicitly passing verifscript, so no cons to me.

@lock9
Copy link
Contributor

lock9 commented Sep 10, 2019

but what are the pros @igormcoelho? I also didn't understand the design.
@neo-project/core if you guys are in agreement, I think we can put this into ready-to-implement, I'm not doing it because I can't really understand the changes required.

@erikzhang erikzhang added the ready-to-implement Issue state: Ready to be implemented or implementation in progress label Sep 11, 2019
@shargon
Copy link
Member

shargon commented Sep 12, 2019

According to Erik's comment #1081 (comment) I think this will be resolved with these refactors

@lock9 lock9 removed the design Issue state - Feature accepted but the solution requires a design before being implemented label Sep 12, 2019
@igormcoelho
Copy link
Contributor Author

According to Erik's comment #1081 (comment) I think this will be resolved with these refactors

Thanks for informing @shargon . It's a good place to fix this 👍

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. ready-to-implement Issue state: Ready to be implemented or implementation in progress vm New features that affect the Neo Virtual Machine or the Interop layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants