use .reverse() + do same for public calls + unit tests #1023
Conversation
| } | ||
| return logs; | ||
| // without the .reverse(), the logs will be in a queue like fashion which is wrong as the kernel processes it like a stack. | ||
| return [execResult.encryptedLogs, ...execResult.nestedExecutions.reverse().flatMap(collectEncryptedLogs)]; |
There was a problem hiding this comment.
Javascript's native reverse is evil af (as most js methods), since it both reverses in place and returns a reference to the reversed array. So, every time you're calling collectEncryptedLogs, you're inadvertently reversing all nested executions.
> a = [1,2,3]
[ 1, 2, 3 ]
> a.reverse()
[ 3, 2, 1 ]
> a
[ 3, 2, 1 ]
Better make a copy of the array before reversing it, like:
return [execResult.encryptedLogs, ...[...execResult.nestedExecutions].reverse().flatMap(collectEncryptedLogs)];There was a problem hiding this comment.
holy mother of god that makes sense - thanks!
There was a problem hiding this comment.
holy mother of god
That's the typical reaction of every developer when faced with js. Reminded me of this "oh-no js" moment I had a while back.
| |---------->fnE (log5) | ||
| |-------->fnF (log6) | ||
| |-------->fnG (log7) | ||
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnE, fnF, fnG fnC, fnD, fnB] |
There was a problem hiding this comment.
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnE, fnF, fnG fnC, fnD, fnB] | |
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnE, fnG, fnF fnC, fnD, fnB] |
At least that's what I understand from the yarn-project/aztec-rpc/src/kernel_prover/kernel_prover.ts.
There was a problem hiding this comment.
yup you are right - in fact the test also returns that - forgot to write my comment correctly thanks!
| fnA () | ||
| |----------> fnB () -> fnC () | ||
| |----------> fnD () -> fnE () | ||
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnB, fnC] |
There was a problem hiding this comment.
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnB, fnC] | |
| Circuits and ACVM process in a DFS + stack like format: [fnA, fnD, fnE, fnB, fnC] |
There was a problem hiding this comment.
ah good catch!
|
ah just realised I need to push to master as opposed to my other branch |
eccvm_recursive_verifier_test measurements (size-512 eccvm recursive verification) Old: 876,214 New: 678,751 The relative performance delta should be much greater for large eccvm instances as this PR removes an nlogn algorithm. This PR resolves issue [#857](AztecProtocol/barretenberg#857) and issue [#1023](AztecProtocol/barretenberg#1023) (single batch mul in IPA) Re: [#1023](AztecProtocol/barretenberg#1023). The code still performs 2 batch muls, but all additional * operator calls have been combined into the batch muls. It is not worth combining both batch muls, as it would require a multiplication operation on a large number of scalar multipliers. In the recursive setting the scalars are bigfield elements - the extra bigfield::operator* cost is not worth combining both batch_mul calls. Additional improvements: removed unneccessary uses of `pow` operator in ipa - in the recursive setting these were stdlib::bigfield::pow calls and very expensive removed the number of distinct multiplication calls in ipa::reduce_verify_internal cycle_scalar::cycle_scalar(stdlib::bigfield) constructor now more optimally constructs a cycle_scalar out of a bigfield element. New method leverages the fact that `scalar.lo` and `scalar.hi` are implicitly range-constrained to remove reundant bigfield constructor calls and arithmetic calls, and the process of performing a scalar multiplication applies a modular reduction to the imput, which makes the explicit call to `validate_scalar_is_in_field` unneccessary --------- Co-authored-by: lucasxia01 <[email protected]>
Description
Stack is basically a queue in a reverse order. So doing what santiago originally did but adding a
.reverse()would work.Added some tests
Please sanity check that the ordering I have written in the tests are similar to what you would expect from ACVM or kernel
Checklist: