Skip to content

refac: remove conditional compilation over DA types#501

Merged
fakedev9999 merged 58 commits intomainfrom
taehoon/refac-wit-exec
May 12, 2025
Merged

refac: remove conditional compilation over DA types#501
fakedev9999 merged 58 commits intomainfrom
taehoon/refac-wit-exec

Conversation

@fakedev9999
Copy link
Copy Markdown
Member

@fakedev9999 fakedev9999 commented Apr 24, 2025

Removes conditional compilation over DA types by introducing WitnessData, WitnessExecutor, WitnessGenerator trait, ethereum and celestia modules.

@ratankaliani ratankaliani force-pushed the taehoon/refac-wit-exec branch from b559abe to ef59271 Compare April 25, 2025 00:50
Comment thread programs/range/celestia/src/main.rs Outdated
Comment thread programs/range/celestia/Cargo.toml Outdated
Comment thread utils/host/src/witness_generation/client.rs Outdated
@ratankaliani ratankaliani force-pushed the taehoon/refac-wit-exec branch from ef59271 to 4b8d6f0 Compare April 25, 2025 01:54
Comment thread utils/host/src/witness_generation/client.rs Outdated
@fakedev9999 fakedev9999 marked this pull request as ready for review April 25, 2025 08:46
@ratankaliani ratankaliani force-pushed the taehoon/refac-wit-exec branch from 05be1c3 to d9a7e16 Compare April 25, 2025 08:49
Comment thread validity/src/proposer.rs Outdated
@fakedev9999 fakedev9999 changed the title refac: introduce WitnessExecutor refac: remove conditional compilation over DA types Apr 25, 2025
Comment thread fault-proof/bin/proposer.rs Outdated
Comment thread utils/celestia/src/host.rs Outdated
Comment thread utils/ethereum/src/host.rs Outdated
@fakedev9999 fakedev9999 force-pushed the taehoon/refac-wit-exec branch from ffdac10 to 7e51ff1 Compare May 6, 2025 08:05
Copy link
Copy Markdown

@ratankaliani ratankaliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_proof_stdin should not be async

Comment thread programs/range/celestia/src/main.rs Outdated
Comment thread programs/range/ethereum/src/main.rs Outdated
Comment thread utils/client/src/witness/executor.rs Outdated
Comment thread utils/client/src/witness/mod.rs Outdated
@fakedev9999 fakedev9999 force-pushed the taehoon/refac-wit-exec branch 2 times, most recently from e83330c to 45c6479 Compare May 7, 2025 08:42
@succinctlabs succinctlabs deleted a comment from github-actions Bot May 8, 2025
Comment thread utils/celestia/host/src/host.rs Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2025

Performance Comparison

Range 2071356~2071361

Metric Base Branch Current PR Diff (%)
Total Instructions 1704959414 1701943538 -0.18%
Oracle Verify Cycles 195682199 195682195 -0.00%
Derivation Cycles 1365411385 1362586288 -0.21%
Block Execution Cycles 7386315 7369762 -0.22%
Blob Verification Cycles 22996050 22745035 -1.09%
Total SP1 Gas 2401726496 2396873930 -0.20%
Cycles per Block 340991882 340388707 -0.18%
Cycles per Transaction 340991882 340388707 -0.18%
BN Pair Cycles 0 0 0.00%
BN Add Cycles 0 0 0.00%
BN Mul Cycles 0 0 0.00%
KZG Eval Cycles 0 0 0.00%
EC Recover Cycles 0 0 0.00%
P256 Verify Cycles 0 0 0.00%

Comment thread scripts/prove/bin/multi.rs Outdated
Comment thread scripts/prove/tests/cycle_count_diff.rs Outdated
Comment thread scripts/prove/bin/multi.rs
Comment thread scripts/prove/tests/cycle_count_diff.rs Outdated
Comment thread scripts/prove/tests/cycle_count_diff.rs Outdated
Comment thread scripts/prove/tests/multi.rs Outdated
Comment thread scripts/prove/tests/multi.rs Outdated
Comment thread scripts/utils/bin/cost_estimator.rs Outdated
Comment thread scripts/utils/bin/cost_estimator.rs Outdated
Comment thread scripts/utils/bin/gen_sp1_test_artifacts.rs Outdated
Comment thread scripts/utils/bin/gen_sp1_test_artifacts.rs Outdated
Comment thread utils/celestia/host/src/host.rs Outdated
Comment thread utils/client/src/witness/mod.rs Outdated
Comment thread utils/proof/src/lib.rs
Comment thread validity/Cargo.toml
Comment thread utils/ethereum/client/src/executor.rs
Copy link
Copy Markdown

@ratankaliani ratankaliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recent changes to kona definitely simplified the diff. Have a few points which should be addressed before merging.

  • Without a celestia feature on the proposer, how will the host be initialized with the CelestiaHost. IIUC you need the celestia feature on the proposer to activate the feature on op-succinct-proof-utils which activates the celestia host.
  • The WitnessExecutor implementations can likely share more code.
  • Is there a way to avoid the marker on the WitnessExecutor trait? That null pattern is typically an anti-pattern.
  • Left some comments about propagating errors when possible using ?, rather than unwrapping.

@fakedev9999
Copy link
Copy Markdown
Member Author

  • Without a celestia feature on the proposer, how will the host be initialized with the CelestiaHost. IIUC you need the celestia feature on the proposer to activate the feature on op-succinct-proof-utils which activates the celestia host.

As replied above, no need to declare features of op-succinct-proof-utils. Passing in feature flag will enable feature in op-succinct-proof-utils.

  • The WitnessExecutor implementations can likely share more code.

Not sure if this can be done without feature flags.

  • Is there a way to avoid the marker on the WitnessExecutor trait? That null pattern is typically an anti-pattern.

The only alternative would be to store actual fields of these types, but there are two structs PreimageWitnessCollector and PreimageStore input as the oracle, which will end up having two structs implementing of WitnessExecutor with different types of oracle, which will lead to a lot of duplicated codes.

  • Left some comments about propagating errors when possible using ?, rather than unwrapping.

Applied!

Comment thread Cargo.toml Outdated
Comment thread utils/build/src/lib.rs Outdated
@fakedev9999 fakedev9999 merged commit 9efcc10 into main May 12, 2025
8 checks passed
@fakedev9999 fakedev9999 deleted the taehoon/refac-wit-exec branch May 12, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants