tx/block: simplify validate methods

[gabrocheleau]

resolves #2726

This PR mainly removes the Transaction.validate overloaded methods, and replaces them with these two:

• an isValid() method that returns a boolean
• a getValidationErrors method that returns the validation error strings

It also simplifies the corresponding Block.validateTransactions methods in the same way, and unifies the naming of a few other block validation methods to conform to the following semantics:

• isValid() returns true if tx is valid, false if not
• validate() throws if invalid
• getValidationErrors() returns an array of error strings

I also suspect those updates will bring performance improvements, although I'm not sure how significant. Currently we were going through all validation checks, populating an errors: string[], and only then returning a boolean if the errorStr: boolean was undefined/false. This is quite wasteful as when only a boolean is return, we can return false as soon as a validation error is detected.

[gabrocheleau]

There was also mention in the #2767 PR comments that we should also take a look at the tx.getMessageToSign method.

  getMessageToSign(hashMessage: false): Uint8Array | Uint8Array[]
  getMessageToSign(hashMessage?: true): Uint8Array

I am less convinced that this simplification would be meaningful. If we strictly want to avoid overloading, we could split it in two methods, namely getMessageToSign and getHashedMessageToSign.

[codecov]

Codecov Report

Merging #2792 (2d3094b) into master (91f7c9d) will increase coverage by 1.37%.
The diff coverage is 99.22%.

Additional details and impacted files

Flag	Coverage Δ
block	89.13% <95.83%> (-0.17%)	⬇️
blockchain	92.50% <ø> (ø)
client	87.08% <ø> (?)
common	97.09% <ø> (ø)
devp2p	?
ethash	∅ <ø> (∅)
evm	66.78% <ø> (ø)
rlp	∅ <ø> (∅)
statemanager	86.35% <ø> (ø)
trie	90.01% <ø> (-0.05%)	⬇️
tx	95.97% <100.00%> (+0.09%)	⬆️
util	85.31% <ø> (ø)
vm	77.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

I'm a bit undecided on this PR, will give this another round of thought.

[holgerd77]

Hey there, had a first looks, two comments.

Also, for getMessageToSign():

Yes, can we please do this, to split this up into two methods getMessageToSign() and getHashedMessageToSign()?

I can remember various confusing discussions (so: library users where confused 😋) around this method, and this would mitigate this, this would be also relevant since this is a very central/sensitive method people use for ledger signing and the like, it's good to reduce confusion here.

And we then also remove this overloading, which is good.

Can you also integrate in this PR? Thanks! 🙂

[holgerd77]

Can we unify the code logic here regarding isValid() and getValidationErrors, so that we do not have to maintain the checks in two different methods, which is prone to errors?

So use getValidatonErrors() from within isValid() and return false there if the error array is popilated.

(just remembering the discussion from yesterday: so this is not relevant from a performance perspective)

[jochem-brouwer]

Yes, and I think the same holds for block.

[holgerd77]

Still commented out code parts.

[gabrocheleau]

Removed

[jochem-brouwer]

Some comments.

In general: shouldn't we generalize this interface? We now have

isValid + getValidationErrors on tx,

isValid + validate + getValidationErrors on block

We could thus also generalize those by writing an internal validation method which validates all rules.isValid loops over these rules via a try catch, if one of the rules throws return false else true. validate just loops over the rules, if one of the methods throws validate then also throws. getValidationErrors loops over the rules via try catch and pushes errors onto an array and finally returns those (or empty array if no errors).

(Alternatively we could also create two arrays of the validation methods, where the validation methods return either true or false and we lookup an error message from the array if we want the error message - this would be slightly more performant since we don't have to create the strings if we throw)

WDYGT?

[gabrocheleau]

Some comments.

In general: shouldn't we generalize this interface? We now have

isValid + getValidationErrors on tx,

isValid + validate + getValidationErrors on block

We could thus also generalize those by writing an internal validation method which validates all rules.isValid loops over these rules via a try catch, if one of the rules throws return false else true. validate just loops over the rules, if one of the methods throws validate then also throws. getValidationErrors loops over the rules via try catch and pushes errors onto an array and finally returns those (or empty array if no errors).

(Alternatively we could also create two arrays of the validation methods, where the validation methods return either true or false and we lookup an error message from the array if we want the error message - this would be slightly more performant since we don't have to create the strings if we throw)

WDYGT?

I think the approach you suggest makes sense, but I doubt it is necessary at this point, as we don't have to my knowledge cases where we have all 3 methods, only two instances where we have "isValid" and "getValidationErrors". I feel it's sufficient to just have the isValid method call this.getValidationErrors and just return errors.length === 0. I've done the same for getTransactionsValidationErrors and transactionAreValid from block. If others feel like your approach would be more useful though I can implement it, just seems a bit overkill at this stage in my view.

[gabrocheleau]

I refactored getMessageToSign(true) and getMessageToSign(false) into two separate methods (getMessageToSign and getHashedMessageToSign respectively) and simplified the code of some validate methods to make the code more DRY.

[holgerd77]

Have given this a branch update, will have another look at this PR now.

[holgerd77]

I have to say I am now totally in love with this PR and all the changes it contain after having a third look with some distance, guess it was also just the amount of things changed here which needed to sink in a bit.

But these new structured naming semantics make so much sense, also (to also tap a bit on my shoulder 😅) I really think that this second overlading removal is such a big win, getHashedMessageToSign() is just so much more explicit and readable than tx.getMessageToSign(true) (where you can literally "read" nothing from about what this call actually does).

So, great great, will approve and merge, thanks for tackling! 🙂 👍

[jochem-brouwer]

I agree here, my proposal would make everything complicated which is not necessary. Great work 🎉

[jochem-brouwer]

Forgot to note: these method names changes are breaking ( @holgerd77 ). But we can (?) release those?

[holgerd77]

Failing tests are unrelated, will merge here.

There seems to be some timeout now in some blockchain tests, @acolytec3 did you actually introduce some additional caching now? 🤔 Might be related (building cache takes to long or something).

[holgerd77]

Forgot to note: these method names changes are breaking ( @holgerd77 ). But we can (?) release those?

Yes, super-breaking for sure, that's why I needed three rounds to let this sink in if it's worth it. But - yes - we do breaking releases anyhow, it's a bit tedious for people but I would now say that this clear semantics (everything *IsValid gives a boolean) is worth it, since this helps a lot on easy-code understanding.

[acolytec3]

Failing tests are unrelated, will merge here.

There seems to be some timeout now in some blockchain tests, @acolytec3 did you actually introduce some additional caching now? 🤔 Might be related (building cache takes to long or something).

Not that I'm aware of. I have some experimental PRs with build caches but nothing that got merged into master that I'm aware of

[jochem-brouwer]

I re-ran the test, it was likely just the runner which was being super slow :)