Conversation
Member
Tristan-Wilson
commented
Jun 18, 2025
Tristan-Wilson
commented
- Introduce ICustomDAProofValidator interface for extensible DA proof validation
- Implement ReferenceDAProofValidator with hash verification and chunk extraction
- Integrate CustomDA validation into OneStepProverHostIo for preimage type 3
- Add comprehensive test coverage for reference validator implementation
- Introduce ICustomDAProofValidator interface for extensible DA proof validation - Implement ReferenceDAProofValidator with hash verification and chunk extraction - Integrate CustomDA validation into OneStepProverHostIo for preimage type 3 - Add comprehensive test coverage for reference validator implementation
gzeoneth
reviewed
Jun 23, 2025
src/osp/OneStepProverHostIo.sol
Outdated
Comment on lines
+33
to
+41
| ICustomDAProofValidator public customDAValidator; | ||
|
|
||
| function setCustomDAValidator( | ||
| ICustomDAProofValidator _validator | ||
| ) external { | ||
| // TODO: Add appropriate access control | ||
| customDAValidator = _validator; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
we don't keep any storage in the OSP contract, so instead I think we have 2 options
- store the var in the rollup and have the OSP read from the rollup
- make this immutable and set in the constructor, but we have to consider how it works with the rollup creator
Member
There was a problem hiding this comment.
I would prefer to make this immutable because this allow more deterministic behavior, otherwise the rollup owner may change the DA contract and make a assertion invalid.
…m/OffchainLabs/nitro-contracts into deterministic-factory-deployments
Since this is a reference implementation only, it's moved into the nitro repo under contracts-local.
Member
Author
|
I've moved the reference implementation into nitro's contracts-local directory. |
…tificate Bug was introduced in 44b2eb4 when refactoring from assembly. The certSize was being read from proof[0:] instead of proof[proofOffset:], causing PROOF_TOO_SHORT errors when the validator tried to use garbage data as the certificate size.
gzeoneth
changed the title
Tristan-Wilson
added a commit
to OffchainLabs/nitro
that referenced
this pull request
Oct 17, 2025
This commit moves the ReferenceDAProofValidator contract and tests from nitro-contracts to contracts-local, as this is a reference implementation that doesn't need to be part of the core nitro-contracts package. The solidity contract was already reviewed in OffchainLabs/nitro-contracts#357 Since the Reference DA contract is now available, this commit activates contract-based certificate validation by uncommenting the ValidateWithContract calls in certificate.go, reference_reader.go, and reference_validator.go. These were previously disabled with TODO comments waiting for contract merge. This commit also includes some changes required for nitro-testnode to work in CustomDA mode with Reference DA. It Ensures contracts are available in Docker builds by copying both contracts/ and contracts-local/ directories. It also adds ReferenceDA signing key to config dump exclusion list to prevent accidental exposure of private keys. This change was merged into the custom-da branch in: #3803 Other changes required that were needed for the standalone daprovider to work with nitro-testnode were: - New parent-chain-node-url and parent-chain-connection-attempts config - L1 client creation in daprovider startup for ReferenceDA mode This change was merged into the custom-da branch in: #3819
Tristan-Wilson
added a commit
to OffchainLabs/nitro
that referenced
this pull request
Oct 17, 2025
This commit moves the ReferenceDAProofValidator contract and tests from nitro-contracts to contracts-local, as this is a reference implementation that doesn't need to be part of the core nitro-contracts package. The solidity contract was already reviewed in OffchainLabs/nitro-contracts#357 Since the Reference DA contract is now available, this commit activates contract-based certificate validation by uncommenting the ValidateWithContract calls in certificate.go, reference_reader.go, and reference_validator.go. These were previously disabled with TODO comments waiting for contract merge. This commit also includes some changes required for nitro-testnode to work in CustomDA mode with Reference DA. It Ensures contracts are available in Docker builds by copying both contracts/ and contracts-local/ directories. It also adds ReferenceDA signing key to config dump exclusion list to prevent accidental exposure of private keys. This change was merged into the custom-da branch in: #3803 Other changes required that were needed for the standalone daprovider to work with nitro-testnode were: - New parent-chain-node-url and parent-chain-connection-attempts config - L1 client creation in daprovider startup for ReferenceDA mode This change was merged into the custom-da branch in: #3819
yahgwai
previously approved these changes
Oct 20, 2025
github-merge-queue bot
pushed a commit
to OffchainLabs/nitro
that referenced
this pull request
Oct 23, 2025
* Add proof enhancer system with customda enhancers This adds infrastructure to enhance one-step proofs with additional data required by the arbitrator, particularly for custom DA systems. The proof enhancer system intercepts one-step proofs that have an enhancement flag set by the arbitrator. When the arbitrator needs additional data that it cannot access directly (like DA certificates or preimage data), it sets this flag along with a marker byte indicating what type of enhancement is needed. The system includes: - ProofEnhancementManager: Routes proofs to appropriate enhancers based on marker bytes - ReadPreimageProofEnhancer: Handles DA preimage read requests (marker 0xDA) - ValidateCertificateProofEnhancer: Handles certificate validation requests (marker 0xDB) Both enhancers retrieve the certificate from the sequencer message stored in the inbox, then use the daprovider.Validator interface to generate the appropriate proofs. This design allows the arbitrator to request DA operations without needing to store large certificates in its limited WASM memory. The enhanced proofs are then sent to the OSP (on-chain prover) which can verify them against the actual DA system's validation logic. * Add more comments explaining proof enhancement * ProofMarker byte type alias * Remove hardcoded values * Move proof enhancer to its own package * Convenience method for creating custom DA proof enhancers * add comments about enhancement flags * Full ReferenceDA implementation This commit moves the ReferenceDAProofValidator contract and tests from nitro-contracts to contracts-local, as this is a reference implementation that doesn't need to be part of the core nitro-contracts package. The solidity contract was already reviewed in OffchainLabs/nitro-contracts#357 Since the Reference DA contract is now available, this commit activates contract-based certificate validation by uncommenting the ValidateWithContract calls in certificate.go, reference_reader.go, and reference_validator.go. These were previously disabled with TODO comments waiting for contract merge. This commit also includes some changes required for nitro-testnode to work in CustomDA mode with Reference DA. It Ensures contracts are available in Docker builds by copying both contracts/ and contracts-local/ directories. It also adds ReferenceDA signing key to config dump exclusion list to prevent accidental exposure of private keys. This change was merged into the custom-da branch in: #3803 Other changes required that were needed for the standalone daprovider to work with nitro-testnode were: - New parent-chain-node-url and parent-chain-connection-attempts config - L1 client creation in daprovider startup for ReferenceDA mode This change was merged into the custom-da branch in: #3819 * Add ProviderType byte to ReferenceDA certificate This shows how different custom DA providers can distinguish themselves by using a byte after the DACertificateMessageHeaderFlag which identifies the certificate as coming from some custom DA system.
github-merge-queue bot
pushed a commit
to OffchainLabs/nitro
that referenced
this pull request
Oct 24, 2025
* Add proof enhancer system with customda enhancers This adds infrastructure to enhance one-step proofs with additional data required by the arbitrator, particularly for custom DA systems. The proof enhancer system intercepts one-step proofs that have an enhancement flag set by the arbitrator. When the arbitrator needs additional data that it cannot access directly (like DA certificates or preimage data), it sets this flag along with a marker byte indicating what type of enhancement is needed. The system includes: - ProofEnhancementManager: Routes proofs to appropriate enhancers based on marker bytes - ReadPreimageProofEnhancer: Handles DA preimage read requests (marker 0xDA) - ValidateCertificateProofEnhancer: Handles certificate validation requests (marker 0xDB) Both enhancers retrieve the certificate from the sequencer message stored in the inbox, then use the daprovider.Validator interface to generate the appropriate proofs. This design allows the arbitrator to request DA operations without needing to store large certificates in its limited WASM memory. The enhanced proofs are then sent to the OSP (on-chain prover) which can verify them against the actual DA system's validation logic. * Add more comments explaining proof enhancement * ProofMarker byte type alias * Remove hardcoded values * Move proof enhancer to its own package * Convenience method for creating custom DA proof enhancers * add comments about enhancement flags * Full ReferenceDA implementation This commit moves the ReferenceDAProofValidator contract and tests from nitro-contracts to contracts-local, as this is a reference implementation that doesn't need to be part of the core nitro-contracts package. The solidity contract was already reviewed in OffchainLabs/nitro-contracts#357 Since the Reference DA contract is now available, this commit activates contract-based certificate validation by uncommenting the ValidateWithContract calls in certificate.go, reference_reader.go, and reference_validator.go. These were previously disabled with TODO comments waiting for contract merge. This commit also includes some changes required for nitro-testnode to work in CustomDA mode with Reference DA. It Ensures contracts are available in Docker builds by copying both contracts/ and contracts-local/ directories. It also adds ReferenceDA signing key to config dump exclusion list to prevent accidental exposure of private keys. This change was merged into the custom-da branch in: #3803 Other changes required that were needed for the standalone daprovider to work with nitro-testnode were: - New parent-chain-node-url and parent-chain-connection-attempts config - L1 client creation in daprovider startup for ReferenceDA mode This change was merged into the custom-da branch in: #3819 * Add ProviderType byte to ReferenceDA certificate This shows how different custom DA providers can distinguish themselves by using a byte after the DACertificateMessageHeaderFlag which identifies the certificate as coming from some custom DA system.
github-merge-queue bot
pushed a commit
to OffchainLabs/nitro
that referenced
this pull request
Oct 24, 2025
* Add proof enhancer system with customda enhancers This adds infrastructure to enhance one-step proofs with additional data required by the arbitrator, particularly for custom DA systems. The proof enhancer system intercepts one-step proofs that have an enhancement flag set by the arbitrator. When the arbitrator needs additional data that it cannot access directly (like DA certificates or preimage data), it sets this flag along with a marker byte indicating what type of enhancement is needed. The system includes: - ProofEnhancementManager: Routes proofs to appropriate enhancers based on marker bytes - ReadPreimageProofEnhancer: Handles DA preimage read requests (marker 0xDA) - ValidateCertificateProofEnhancer: Handles certificate validation requests (marker 0xDB) Both enhancers retrieve the certificate from the sequencer message stored in the inbox, then use the daprovider.Validator interface to generate the appropriate proofs. This design allows the arbitrator to request DA operations without needing to store large certificates in its limited WASM memory. The enhanced proofs are then sent to the OSP (on-chain prover) which can verify them against the actual DA system's validation logic. * Add more comments explaining proof enhancement * ProofMarker byte type alias * Remove hardcoded values * Move proof enhancer to its own package * Convenience method for creating custom DA proof enhancers * add comments about enhancement flags * Full ReferenceDA implementation This commit moves the ReferenceDAProofValidator contract and tests from nitro-contracts to contracts-local, as this is a reference implementation that doesn't need to be part of the core nitro-contracts package. The solidity contract was already reviewed in OffchainLabs/nitro-contracts#357 Since the Reference DA contract is now available, this commit activates contract-based certificate validation by uncommenting the ValidateWithContract calls in certificate.go, reference_reader.go, and reference_validator.go. These were previously disabled with TODO comments waiting for contract merge. This commit also includes some changes required for nitro-testnode to work in CustomDA mode with Reference DA. It Ensures contracts are available in Docker builds by copying both contracts/ and contracts-local/ directories. It also adds ReferenceDA signing key to config dump exclusion list to prevent accidental exposure of private keys. This change was merged into the custom-da branch in: #3803 Other changes required that were needed for the standalone daprovider to work with nitro-testnode were: - New parent-chain-node-url and parent-chain-connection-attempts config - L1 client creation in daprovider startup for ReferenceDA mode This change was merged into the custom-da branch in: #3819 * Add ProviderType byte to ReferenceDA certificate This shows how different custom DA providers can distinguish themselves by using a byte after the DACertificateMessageHeaderFlag which identifies the certificate as coming from some custom DA system.
Sherlock AI FindingsThe automated tool identified the following potential security issues in the codebase. Please review the details for each issue in the linked dashboard.
Next Steps: Review the linked issues in the dashboard and address high-severity bugs first. Contact the team if you need assistance. Full report available at: https://ai.sherlock.xyz/runs/dcdc771b-a7b1-4659-b85b-cf40c69cf37e |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.