-
Notifications
You must be signed in to change notification settings - Fork 69
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
refactor!: use big endian and simplify transcript logic #1801
Conversation
|
Bug (that I introduce in this PR)Let me document a minimally reproducible bug (of my own code) that blocks me for days: import "forge-std/Test.sol";
import { Transcript as T } from "../src/libraries/Transcript.sol";
library Transcript {
struct TranscriptData {
bytes transcript;
}
function getAndAppendChallenge(TranscriptData memory self) internal pure returns (uint256) {
bytes memory transcript = self.transcript;
uint256 ret = uint256(keccak256(transcript)) % BN254.R_MOD;
assembly {
let len := mload(transcript)
let newLen := add(len, 32)
let dataPtr := add(transcript, 0x20)
mstore(transcript, newLen)
mstore(add(dataPtr, len), ret)
}
return ret;
}
}
contract Whatever is Test {
// run with `forge test --mt whatever -vv`
function test_whatever() external {
T.TranscriptData memory transcript;
transcript.transcript = abi.encodePacked(
transcript.transcript,
uint256(0x1234)
);
// HERE !! try to comment and uncomment
console2.log("hi");
uint256 chal = T.getAndAppendChallenge(transcript);
console2.log("chal: %x", chal);
console2.logBytes(transcript.transcript);
revert(); // deliberately revert to print logs
}
} so comment out the
uncomment it, you would get the correct/expected transcript bytes:
DiagonsisI couldn't wrap my head around this behavior, and only after went into the debugger and look at stack and memory content and stepping through instruction by instruction do I realize the problem comes from: my inline assembly code in Reason why injecting a that Knowing my problem, I find this StackOverflow question that discussed the same issue. Solutionsimply add one line to update the free memory pointer towards the end: assembly {
// ...
// update free memory pointer since we extend the dynamic array
// to prevent potential overwrite
mstore(0x40, add(mload(0x40), 32))
} TakeawayBe careful about memory safety when editing dynamic memory using inline assembly. |
Re benchmarks: When I comment these three lines the gas consumption is around 170k. When I do the same in b3ec621 I get 90k. So there is around a 80k gas leakage in this part of the code. Weird. |
@alxiong : #1816 created so that we can come back to it later. |
Also found this on Solidity doc about safe memory management. Apparently I didn't update myself with this warning. Lessons learned. |
* fix: bug on lagrange coeff computation (#1639) * Fix the edge case when zeta is one of the evaluation domain elements, and our lagrange coefficient (and also pi_poly_eval) is computed incorrectly. Tests are also added for these rare edge cases. * Update script to run gas benchmarks (#1769) * Update script to run gas benchmarks. Store gas benchmarks for this commit. * Update lint.yml as CI is complaining. * Use addmod in function _computeLinPolyConstantTerm. (#1770) * Point to branch 'commonprefix-patch of solidity-bn254 repository * use forge snapshot for gas benchmark instead * chore: update hotshot to 0.1.60, most jf dep to 0.4.5 (#1788) * refactor!: use big endian and simplify transcript logic (#1801) * fix transcript and tests * fix computeChallenges in verifier * update gas benchmark * further improve and add comment to inline assembly * Fix comment in `_linearizationScalarsAndBases` (#1812) * Fix comment in _linearizationScalarsAndBases. * Update contracts/src/libraries/PlonkVerifier.sol Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> * refactor: add G2 point from SRS to verifying key and transcript (#1819) * add G2 from SRS to VK and append to Transcript * update with jf * Remove redundant code (#1821) * Remove unused function _batchVerifyOpeningProofs. * Remove redundant variable sumEval. * attempt: free disk space to avoid ci failure --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> * Gas optimization for evaluatePiPoly (#1822) * Gas optimization for evaluatePiPoly * add credit --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> * improve challenge generation (#1829) * use state-approach for squeeze domain-separation * memory align word size * update with merged jf * feat: Efficient computeChallenges in pure assembly (#1831) * rewrite computeChallenges in pure assembly * add more comments * refactor!: change publicInputs from dynamic to fixed array (#1835) * change publicInputs from dynamic to fixed array * Remove redundant fields in struct EvalDomain. (#1840) * Remove redundant field EvalDomain.groupGenInv * Remove redundant field EvalDomain.size. * address comments --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> * Remove redundant transcript files. (#1836) * Use of constants COSET_K1,...,COSET_K4. * Put Transcript.sol and Transcript.t.sol files in some legacy folder. * Precompute domain elements (#1860) * Remove unused domain sizes. * Hardcode domain elements. * removed domain.groupGen and localDomainElements. --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> * feat!: fully assemblify preparePcsInfo() and verifyOpeningProof (#1844) * perf: reuse free mem for each chal (#1943) * perf: avoid carrying around commScalars and commBases (#1940) * avoid carrying around commScalars and commBases * address jakov comment on L01 * Fix justfile * Small fixes. Generate bindings. * More fixes --------- Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com>
Part of #1739
Closes #1797
This PR:
jf-plonk
which modify thePlonkTranscript
logicg1Serialize
, simply concatenate two base fields to the transcript byte array_computeChallenges()
to reflect the same changeGas benchmark
I'm not 100% sure why the cost of PlonkVerifier goes down but the LightClient cost goes up.
(see if you have any idea @philippecamacho ?)