-
Notifications
You must be signed in to change notification settings - Fork 40
Enhance the SmtLib to support configurable hashing algorithm #402
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Make SmtLib configurable for the hashing algorithm, and add support for keccak256
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
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.
Pull request overview
This PR enhances the SmtLib library to support configurable hashing algorithms while maintaining backward compatibility with the existing Poseidon hash implementation. The primary motivation is to enable use cases where ZKPs are not required, such as the notary-based privacy token implementation in LFDT's Paladin project.
Key Changes:
- Introduces an
IHasherinterface to enable pluggable hash implementations - Implements a Keccak256-based hasher as an alternative to Poseidon
- Maintains Poseidon as the default hasher for backward compatibility
- Adds comprehensive test coverage for Keccak256 hasher with cross-verification against js-merkletree
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/interfaces/IHasher.sol | Defines the pluggable hasher interface with hash2 and hash3 functions |
| contracts/lib/hash/KeccakHasher.sol | Implements IHasher using Keccak256 for non-ZKP use cases |
| contracts/lib/SmtLib.sol | Adds hasher configurability with storage fields and conditional hashing logic |
| contracts/test-helpers/SmtLibKeccakTestWrapper.sol | Test wrapper that initializes SMT with Keccak256 hasher |
| helpers/DeployHelper.ts | Adds useKeccakHashing parameter to deploy the appropriate test wrapper |
| test/smtLib/smtLib.keccak.test.ts | Comprehensive test suite validating Keccak256 hasher behavior |
| package.json | Updates postinstall script to use npx for patch-package |
Comments suppressed due to low confidence (1)
helpers/DeployHelper.ts:427
- When deploying the Keccak hasher variant (useKeccakHashing=true), the deployment still requires deploying the Poseidon libraries (lines 421-427) and linking them to SmtLib. This creates unnecessary deployment overhead and gas costs for users who only want to use Keccak hashing. The PR description acknowledges this as "a rather big annoyance" but this implementation preserves the problem rather than solving it. Consider creating a separate SmtLib variant without Poseidon dependencies for Keccak-only use cases.
this.log("deploying poseidons...");
const [poseidon2Elements, poseidon3Elements] = await deployPoseidons([2, 3]);
const smtLib = await this.deploySmtLib(
await poseidon2Elements.getAddress(),
await poseidon3Elements.getAddress(),
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint256[45] __gap; | ||
| bool isCustomHasherSet; | ||
| IHasher hasher; |
Copilot
AI
Dec 17, 2025
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.
The storage gap has been reduced from 45 to what should be 43 slots, but the comment still says uint256[45] __gap. After adding isCustomHasherSet (bool, typically 1 slot when packed) and hasher (address reference, 1 slot), the gap should be reduced to maintain the same total storage footprint. The gap array size declaration needs to be updated to uint256[43] to account for the two new fields, ensuring storage layout compatibility for upgradeable contracts.
| * @dev Sets custom hashers for the SMT. MUST be called before any other SMT operations. | ||
| * @param customerHasher IHasher implementation to be used for hashing. | ||
| */ | ||
| function setHasher(Data storage self, IHasher customerHasher) external { |
Copilot
AI
Dec 17, 2025
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.
The setHasher function lacks critical validations: (1) It should check that the hasher address is not zero to prevent setting an invalid hasher, (2) It should verify that the SMT has not been used yet (i.e., rootEntries.length == 1, since initialization adds one root entry), as changing the hasher after operations have been performed would invalidate all existing tree nodes and make the tree inconsistent. The documentation states it "MUST be called before any other SMT operations" but this is not enforced in code.
| function setHasher(Data storage self, IHasher customerHasher) external { | |
| function setHasher(Data storage self, IHasher customerHasher) external { | |
| require(address(customerHasher) != address(0), "Invalid hasher"); | |
| require(self.rootEntries.length == 1, "Hasher must be set before SMT usage"); |
| * @param customerHasher IHasher implementation to be used for hashing. | ||
| */ | ||
| function setHasher(Data storage self, IHasher customerHasher) external { | ||
| self.isCustomHasherSet = true; | ||
| self.hasher = customerHasher; |
Copilot
AI
Dec 17, 2025
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.
The parameter name has a typo: "customerHasher" should be "customHasher". This naming inconsistency could cause confusion when reviewing or using the API.
| * @param customerHasher IHasher implementation to be used for hashing. | |
| */ | |
| function setHasher(Data storage self, IHasher customerHasher) external { | |
| self.isCustomHasherSet = true; | |
| self.hasher = customerHasher; | |
| * @param customHasher IHasher implementation to be used for hashing. | |
| */ | |
| function setHasher(Data storage self, IHasher customHasher) external { | |
| self.isCustomHasherSet = true; | |
| self.hasher = customHasher; |
This is useful in situations when the SMT construct is used without needing ZKPs, for instance when used by the notary based privacy token implementation in LFDT's Paladin project.
The goal is first of all making the current Poseidon hash the default, such that existing clients using the
SmtLib.sollibrary does not need to change the source code at all after the dependency upgrade.Via a standard interface
IHasher, a new pluggable implementation can be added and configured on the SmtLib library.An implementation of the
IHasherinterface for thekeccak256algorithm is also provided.The unit tests are cross-checked with unit tests in this PR: iden3/js-merkletree#53
Discussion
Due to the need for backward compatibility, we need to make the Poseidon hashing the default. In practice, this means the
SmtLib.solpreserves the dependencies on the Poseidon libraries. When I use the keccak hasher, even though I wouldn't have any use for the Poseidon libraries, I would still have to deploy them in order to use theSmtLiblibrary.This seems to be a rather big annoyance. So the question for discussion is whether it's worth building it a bit differently so that the Poseidon capabilities are NOT available by default. Such that when I use keccak, I don't need to bother with deploying the Poseidon libraries. On the other hand, this means the existing usage of the SmtLib library needs to be updated to explicitly configure the hashing to be Poseidon. But it's only one call to set the hasher: