fix: address issues (#773)

Please be sure to look over the pull request guidelines here:
https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

# Please go through the following checklist
- [x] The PR title and commit messages adhere to guidelines here:
https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md.
In particular `!` is used if and only if at least one breaking change
has been introduced.
- [x] I have run the ci check script with `source
scripts/run_ci_checks.sh`.
- [x] I have run the clean commit check script with `source
scripts/check_commits.sh`, and the commit history is certified to follow
clean commit guidelines as described here:
https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/COMMIT_GUIDELINES.md
- [x] The latest changes from `main` have been incorporated to this PR
by simple rebase if possible, if not, then conflicts are resolved
appropriately.

# Rationale for this change
We need to address the following issues. 
<!--
Why are you proposing this change? If this is already explained clearly
in the linked issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.

 Example:
 Add `NestedLoopJoinExec`.
 Closes #345.

Since we added `HashJoinExec` in #323 it has been possible to do
provable inner joins. However performance is not satisfactory in some
cases. Hence we need to fix the problem by implement
`NestedLoopJoinExec` and speed up the code
 for `HashJoinExec`.
-->

# What changes are included in this PR?
- add to docs for plan name skipping with respect to what is returned
- fix typo in Doc
- replace `mulmod` with `mul` in `make_transcript`
<!--
There is no need to duplicate the description in the ticket here but it
is sometimes worth providing a summary of the individual changes in this
PR.

Example:
- Add `NestedLoopJoinExec`.
- Speed up `HashJoinExec`.
- Route joins to `NestedLoopJoinExec` if the outer input is sufficiently
small.
-->

# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?

Example:
Yes.
-->
N/A.

Author: iajoiner

solidity/src/base/Errors.sol

@@ -8,6 +8,9 @@ uint32 constant ERR_INVALID_EC_ADD_INPUTS = 0x765bcba0;
 uint32 constant ERR_INVALID_EC_MUL_INPUTS = 0xe32c7472;
 /// @dev Error code for when ECPAIRING inputs are invalid.
 uint32 constant ERR_INVALID_EC_PAIRING_INPUTS = 0x4385b511;
+/// @dev Error code for commitment array having odd length which is impossible
+/// since each commitment is 2 elements.
+uint32 constant ERR_COMMITMENT_ARRAY_ODD_LENGTH = 0x88acadef;
 /// @dev Error code for when the size of a sumcheck proof is incorrect.
 uint32 constant ERR_INVALID_SUMCHECK_PROOF_SIZE = 0x3f889a17;
 /// @dev Error code for when the evaluation of a round in a sumcheck proof does not match the expected value.
@@ -58,6 +61,9 @@ library Errors {
     error InvalidECMulInputs();
     /// @notice Error thrown when the inputs to the ECPAIRING precompile are invalid.
     error InvalidECPairingInputs();
+    /// @notice Error code for commitment array having odd length which is impossible
+    /// since each commitment is 2 elements.
+    error CommitmentArrayOddLength();
     /// @notice Error thrown when the size of a sumcheck proof is incorrect.
     error InvalidSumcheckProofSize();
     /// @notice Error thrown when the evaluation of a round in a sumcheck proof does not match the expected value.

solidity/src/proof_exprs/ProofExpr.pre.sol

@@ -31,7 +31,7 @@ library ProofExpr {
     ///   length equal to the columns in the expression
     /// ##### Return Values
     /// * `expr_ptr_out` - pointer to the remaining expression after consuming the proof expression
-    /// * `eval` - the evaluation of the result of this expression. Cirically, this resulting evaluation must be guarenteed to be
+    /// * `eval` - the evaluation of the result of this expression. Critically, this resulting evaluation must be guarenteed to be
     ///   the correct evaluation of a column with the same length as the columns in the expression. Every column has implicit infinite length
     ///   but is padded with zeros. This is guarenteed to match the length of the chi column, and varients must be designed to handle this.
     /// ##### Proof Plan Encoding

solidity/src/verifier/PlanUtil.sol

@@ -25,7 +25,7 @@ library PlanUtil {
     /// @dev     * length of output column name (uint64)
     /// @dev     * output column name (variable length)
     /// @param __plan The calldata pointer to the plan.
-    /// @return __planOut The updated pointer after skipping names.
+    /// @return __planOut The updated pointer after skipping names which points to the actual plan itself. i.e., the AST without any names.
     function __skipPlanNames(bytes calldata __plan) external pure returns (bytes calldata __planOut) {
         assembly {
             function skip_plan_names(plan_ptr) -> plan_ptr_out {

solidity/src/verifier/Verifier.pre.sol

@@ -454,7 +454,7 @@ library Verifier {
                 append_array(transcript_ptr, table_lengths_ptr)
 
                 let commitment_len := mload(commitments_ptr)
-                mstore(commitments_ptr, mulmod(commitment_len, 2, MODULUS))
+                mstore(commitments_ptr, mul(commitment_len, 2))
                 append_array(transcript_ptr, commitments_ptr)
                 mstore(commitments_ptr, commitment_len)
 
@@ -519,7 +519,10 @@ library Verifier {
                 verify_result_evaluations(result_ptr, evaluation_point_ptr, evaluations_ptr)
             }
 
-            mstore(__commitments, div(mload(__commitments), 2))
+            // Revert if the commitments array has an odd length
+            let commitments_len := mload(__commitments)
+            if mod(commitments_len, 2) { err(ERR_COMMITMENT_ARRAY_ODD_LENGTH) }
+            mstore(__commitments, div(commitments_len, 2))
             verify_query(__result.offset, __plan.offset, __proof.offset, __tableLengths, __commitments)
         }
     }

solidity/test/base/Errors.t.sol

@@ -27,6 +27,13 @@ contract ErrorsTest is Test {
         Errors.__err(ERR_INVALID_EC_PAIRING_INPUTS);
     }
 
+    /// forge-config: default.allow_internal_expect_revert = true
+    function testErrorCommitmentArrayOddLength() public {
+        assert(Errors.CommitmentArrayOddLength.selector == bytes4(ERR_COMMITMENT_ARRAY_ODD_LENGTH));
+        vm.expectRevert(Errors.CommitmentArrayOddLength.selector);
+        Errors.__err(ERR_COMMITMENT_ARRAY_ODD_LENGTH);
+    }
+
     /// forge-config: default.allow_internal_expect_revert = true
     function testErrorInvalidSumcheckProofSize() public {
         assert(Errors.InvalidSumcheckProofSize.selector == bytes4(ERR_INVALID_SUMCHECK_PROOF_SIZE));