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

throw exception on null scriptcontainer #2956

Closed
wants to merge 1 commit into from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Nov 9, 2023

Hi @shargon, I am sorry if this makes you think it is a duplicate pr, i just think #2953 might be better to be solved in this way.

  1. since it will false or exception, there is no need to perform any check or execution at all, a waste of CPU time.

  2. it should be an exception instead of a false, the same logic for checking the hashOrPubkey, it should be consistent.

Thus, I do the null check in the firstline of the CheckWitness method instead of in CheckWitnessInternal, since this check will be done anyway if its null.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this you always will check if it's null, with the previous version you only check if it's null when is not a transaction, so we have one condition less and it doesn't affect the tps.

since it will false or exception, there is no need to perform any check or execution at all, a waste of CPU time.

I prefer to waste cpu in neo-express than in mainnet

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 10, 2023

Like this you always will check if it's null, with the previous version you only check if it's null when is not a transaction, so we have one condition less and it doesn't affect the tps.

since it will false or exception, there is no need to perform any check or execution at all, a waste of CPU time.

I prefer to waste cpu in neo-express than in mainnet

Well, its exactly wasting mainnet CPU time. Fair enough, close if you dont like.

@Jim8y Jim8y closed this Nov 10, 2023
@Jim8y Jim8y deleted the null-scriptcontainer branch November 10, 2023 07:52
UInt160 hash = hashOrPubkey.Length switch
{
20 => new UInt160(hashOrPubkey),
33 => Contract.CreateSignatureRedeemScript(ECPoint.DecodePoint(hashOrPubkey, ECCurve.Secp256r1)).ToScriptHash(),
_ => throw new ArgumentException(null, nameof(hashOrPubkey))
_ => throw new ArgumentException("Invalid hashOrPubKey", nameof(hashOrPubkey))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will crate another pr, this one is mainly about the exception.

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.

2 participants