-
Notifications
You must be signed in to change notification settings - Fork 3
Starknetv0.11 Ready for review #21
Changes from 11 commits
a8c077a
9a6ec46
812f5db
fdc9ece
c1eca9c
5a98fb6
75a7139
f39e618
21a40b0
e2b70e9
79157f6
6a29cf1
68c55e6
2ed31fd
772fa14
48e462d
e68f642
1135443
855a361
2a66654
4502104
d2bf5f0
f58aa9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,14 @@ yarn install | |
cp .env.example .env | ||
npx hardhat run <--network yournetwork > scripts/deploy.ts | ||
``` | ||
SNL1ResolverStub.sol inherits from SNStateProofVerifier.sol. On goerli Pedersen hash contract is already deployed at 0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1 | ||
|
||
SNL1ResolverStub.sol inherits from SNStateProofVerifier.sol and has been deployed to Goerli at 0xBB49c34D4d92aC3207d589657fAC14186a470116 | ||
|
||
On goerli Pedersen hash contract is already deployed at 0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1 | ||
|
||
Poseidon3(starkewares version) contracts EVM code has ben generated from the following repo and deployed on Goerli at 0x84d43a8cbEbF4F43863f399c34c06fC109c957a4. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we add links to goerli etherscan to these contracts' addresses? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here: could we verify this contract? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pedersen contract is not so simple to verify, I have created an issue here -> #22 |
||
|
||
https://github.com/NethermindEth/circomlibjs/ | ||
|
||
## Run contract tests | ||
From the root folder run the following | ||
|
@@ -60,4 +67,4 @@ Help taken from existing implementaion of optimism related solution at https://g | |
|
||
|
||
# Disclaimer | ||
None of the contracts have been audited and shoud not be used in production. | ||
None of the contracts have been audited and should not be used in production. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this file as this is not required. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,16 +42,17 @@ struct StarknetProof { | |
// includes contract proof and state/storage proof for a partciular starknet block | ||
struct StarknetCompositeStateProof { | ||
int256 blockNumber; | ||
uint256 classCommitment; | ||
ContractData contractData; | ||
StarknetProof[] contractProofArray; | ||
StarknetProof[] storageProofArray; | ||
} | ||
|
||
interface IStarknetResolverService { | ||
function addr(bytes32 node) | ||
external | ||
view | ||
returns (StarknetCompositeStateProof memory proof); | ||
interface PoseidonHash3 { | ||
// this is the hades permutation function. TODO update the name when goerli eth is not so expensive | ||
function poseidon( | ||
uint256[3] calldata input | ||
) external view returns (uint256[3] calldata); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit surprised by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not so sure about that, you can use it for contract to contract calls and it works fine, its a little bit cheaper I think(but could be wrong) and one should be aware of the downsides(immutability etc etc). But you are right that that is not the convention and probably not worth the confusion it will cause. I have updated it to memory. Interesting question in general though. Happy to be corrected on this understanding |
||
} | ||
|
||
// Starknet Core Contract Minimal Interface | ||
|
@@ -76,7 +77,10 @@ contract SNStateProofVerifier is | |
{ | ||
uint256 private constant BIG_PRIME = | ||
3618502788666131213697322783095070105623107215331596699973092056135872020481; | ||
uint256 private constant FELT_FOR_STARKNET_STATE_V0 = | ||
28355430774503553497671514844211693180464; // short_str_to_felt for "STARKNET_STATE_V0" | ||
PedersenHash public pedersen; | ||
PoseidonHash3 public poseidon; | ||
StarknetCoreContract public starknetCoreContract; | ||
|
||
/// @custom:oz-upgrades-unsafe-allow constructor | ||
|
@@ -86,28 +90,25 @@ contract SNStateProofVerifier is | |
|
||
function initialize( | ||
address pedersenAddress, | ||
address poseidonAddress, | ||
address _starknetCoreContractAddress | ||
) public initializer { | ||
pedersen = PedersenHash(pedersenAddress); | ||
poseidon = PoseidonHash3(poseidonAddress); | ||
starknetCoreContract = StarknetCoreContract( | ||
_starknetCoreContractAddress | ||
); | ||
__Ownable_init(); | ||
__UUPSUpgradeable_init(); | ||
} | ||
|
||
function _authorizeUpgrade(address newImplementation) | ||
internal | ||
virtual | ||
override | ||
onlyOwner | ||
{} | ||
|
||
function hashForSingleProofNode(StarknetProof memory proof) | ||
private | ||
view | ||
returns (uint256) | ||
{ | ||
function _authorizeUpgrade( | ||
address newImplementation | ||
) internal virtual override onlyOwner {} | ||
|
||
function hashForSingleProofNode( | ||
StarknetProof memory proof | ||
) private view returns (uint256) { | ||
uint256 hashvalue = 0; | ||
if (proof.nodeType == NodeType.BINARY) { | ||
hashvalue = hash( | ||
|
@@ -123,6 +124,27 @@ contract SNStateProofVerifier is | |
return hashvalue; | ||
} | ||
|
||
// based on https://docs.starknet.io/documentation/architecture_and_concepts/Hashing/hash-functions/ | ||
function poseidonHashMany( | ||
uint256[] memory elems | ||
) public view returns (uint256) { | ||
uint256[3] memory state = [uint256(0), uint256(0), uint256(0)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe I'm missing something, but I feel that the code here never updates There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, it's probably updated by the function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that is correct |
||
|
||
for (uint256 i = 0; i < elems.length / 2; i++) { | ||
state[0] = (state[0] + elems[2 * i]) % BIG_PRIME; | ||
state[1] = (state[1] + elems[2 * i + 1]) % BIG_PRIME; | ||
state = poseidon.poseidon(state); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternative to avoid division and multiplication: for (uint256 i = 0; i < elems.length; i += 2) {
state[0] = (state[0] + elems[i]) % BIG_PRIME;
state[1] = (state[1] + elems[i + 1]) % BIG_PRIME;
state = poseidon.poseidon(state);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but with (i < elems.length -1) but great suggestion, thanks Massil |
||
|
||
uint256 rem = elems.length % 2; | ||
if (rem == 1) { | ||
state[0] = (state[0] + elems[elems.length - 1]) % BIG_PRIME; | ||
} | ||
state[rem] = (state[rem] + (1)) % BIG_PRIME; | ||
|
||
return poseidon.poseidon(state)[0]; | ||
} | ||
|
||
// this functions connects the contract state root with value of leaf node in the contract proof. | ||
// state_hash = H(H(H(class_hash, contract_root), contract_nonce), RESERVED) | ||
function stateHash( | ||
|
@@ -147,11 +169,10 @@ contract SNStateProofVerifier is | |
return hashes[0]; | ||
} | ||
|
||
function convertToBytes(uint256 x, uint256 y) | ||
private | ||
pure | ||
returns (bytes memory) | ||
{ | ||
function convertToBytes( | ||
uint256 x, | ||
uint256 y | ||
) private pure returns (bytes memory) { | ||
bytes memory b = new bytes(64); | ||
assembly { | ||
mstore(add(b, 32), x) | ||
|
@@ -168,6 +189,7 @@ contract SNStateProofVerifier is | |
// Only supports verifiying a proof for a single storage variable value. | ||
function verifiedStorageValue( | ||
int256 blockNumber, | ||
uint256 classCommitment, | ||
ContractData calldata contractData, | ||
StarknetProof[] calldata contractProofArray, | ||
StarknetProof[] calldata storageProofArray | ||
|
@@ -208,24 +230,23 @@ contract SNStateProofVerifier is | |
contractData.hashVersion | ||
); | ||
|
||
uint256 storageVarValue = verifyProof( | ||
uint256 storageVarValue = verifyStorageProof( | ||
contractData.contractStateRoot, | ||
contractData.storageVarAddress, | ||
storageProofArray | ||
); | ||
|
||
// the contract proof has to be verified against the state root committed on L1 in the Starknet Core Contract | ||
uint256 stateRootCoreHash = starknetCoreContract.stateRoot(); | ||
|
||
require( | ||
_stateHash != 0, | ||
"stateroot hash is not fetched properly! revert" | ||
); | ||
|
||
// the contract proof has to be verified against the state root committed on L1 in the Starknet Core Contract | ||
uint256 expectedStateHash = verifyProof( | ||
stateRootCoreHash, | ||
starknetCoreContract.stateRoot(), | ||
contractData.contractAddress, | ||
contractProofArray | ||
contractProofArray, | ||
classCommitment | ||
); | ||
|
||
require( | ||
|
@@ -248,15 +269,48 @@ contract SNStateProofVerifier is | |
return aExtracted == b; | ||
} | ||
|
||
// overloaded/wrapper function around verifyProof, used to verify storage proof array where the class commitment does not apply | ||
function verifyStorageProof( | ||
uint256 rootHash, | ||
uint256 path, | ||
StarknetProof[] calldata proofArray | ||
) public view returns (uint256 value) { | ||
// classCommitment is 0 means we are verifying storage proof array | ||
return verifyProof(rootHash, path, proofArray, 0); | ||
} | ||
|
||
// A generic method to verify a proof against a root hash and a path. | ||
function verifyProof( | ||
uint256 rootHash, | ||
uint256 path, | ||
StarknetProof[] calldata proofArray | ||
StarknetProof[] calldata proofArray, | ||
uint256 classCommitment // 0 means no class commitment | ||
) public view returns (uint256 value) { | ||
uint256 expectedHash = rootHash; | ||
int256 pathBitIndex = 250; // start from the MSB bit index | ||
if (classCommitment > 0) { | ||
// https://docs.starknet.io/documentation/architecture_and_concepts/State/starknet-state/ | ||
require( | ||
proofArray.length > 0, | ||
"proofs must have atleast one element!" | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we move the same check below above the if statement, we can remove this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
uint256 calculatedContractStateRoot = hashForSingleProofNode( | ||
proofArray[0] | ||
); // the hash of first element in the proof array is the contract state root | ||
uint256[] memory poseidonInput = new uint256[](3); | ||
poseidonInput[0] = FELT_FOR_STARKNET_STATE_V0; | ||
poseidonInput[1] = calculatedContractStateRoot; | ||
poseidonInput[2] = classCommitment; | ||
|
||
uint256 calculateStateCommitment = poseidonHashMany(poseidonInput); | ||
require( | ||
calculateStateCommitment == expectedHash, | ||
"calculated state commitment doesn't match with the expected state commitment!" | ||
); | ||
// since we have already verified the first element in the proof array correctly hashes up to the state commitment, we can assume the hash of first element in the proof array is correct. | ||
expectedHash = calculatedContractStateRoot; | ||
} | ||
require( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can move this check before the if statement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
proofArray.length > 0, | ||
"proof array must have atleast one element." | ||
|
@@ -296,7 +350,6 @@ contract SNStateProofVerifier is | |
expectedHash = proof.edgeProof.childHash; | ||
int256 edgePathLength = int256(proof.edgeProof.length); | ||
pathBitIndex -= edgePathLength; | ||
console.log("pathBitIndex", uint256(pathBitIndex)); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,13 @@ contract StarknetCoreContractStub { | |
*/ | ||
function stateRoot() external view returns (uint256) { | ||
return | ||
3321080970052968843805320357911538392446265524314601848332135665594593955343; | ||
2793869633745137896926484933765493995916819896677591814984705621839603171245; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what the meaning of this value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a comment at the top of the file mentioning its only to be used in tests. I have moved it into mocks folder to make it more explicit. Its mock contract used only unit tests |
||
} | ||
|
||
/** | ||
Returns the current block number. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the comment is not accurate. what does correspond to the block number 789146? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated the comment |
||
*/ | ||
function stateBlockNumber() external view returns (int256) { | ||
return 595364; | ||
return 789146; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we verify this contract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 0xBB49c34D4d92aC3207d589657fAC14186a470116 has been verified but this will change due to the changes as part of this review