RollupParams refactor + STR 478 functional test for full node #371
Conversation
There was a problem hiding this comment.
@delbonis I'm moving part of the conversation from issue #363 here, as it is also relevant to this discussion.
Currently, the RollupParams are passed as input to the ZKVM. The hash of these parameters is used as a public parameter, anchoring all proofs to the RollupParams. This structure ensures that all logic operates with reference to these parameters.
There are three key areas in the provable path where RollupParams are used:
In the Blockspace Proof: Generating tx_filter_rules and validating the committed batch information according to the CredRule
In the CL-STF: Validating block credentials according to the CredRule
In the Checkpoint Proof: Validating the previous checkpoint proof using the verifying key (VK) found in RollupParams
Considerations:
Parameter Size & Serialization:
Currently, the size of the RollupParams is manageable, so the cost of serialization and deserialization isn't excessive. However, it might be beneficial to introduce a constant like RollupParams::DEVNET (similar to bitcoin::params::MAINNET) and use it directly in the guest code, reducing the overhead.
The challenge here is that RollupParams includes the verifying key (VK), which creates a cyclic dependency. One possible solution is to remove the VK from RollupParams and create a separate RollupParamsWithVk struct. Alternatively, the VK could be set to a placeholder (e.g., 0x00) in the guest code and updated with the correct VK later on.
Prover Efficiency:
The tx_filter_rules generation is quite resource-intensive for the prover, consuming ~9 million cycles. In devnet, where L1 blocks are produced every 30 seconds, this becomes more impactful.
Given that tx_filter_rules change infrequently, it might make sense to cache them somewhere, perhaps in the ChainState.
This approach could help optimize prover performance.
Flamegraph Profile Info of processing mainnet block 40321.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #371 +/- ##
==========================================
- Coverage 57.35% 57.26% -0.10%
==========================================
Files 255 255
Lines 26953 26965 +12
==========================================
- Hits 15459 15441 -18
- Misses 11494 11524 +30
|
|
@prajwolrg Would definitely want to cache it in the chainstate somehow. We also would be able to reuse a bunch of state across an epoch, and that would be used across a range of L1 blocks since we will only do the L1 checkin at the end of the epoch. |
remove unwanted params func-test: fix rollup params config func-test: add full node sync datatool, tx-parser: remove addr from DepositTxConfig and add it to DepositTxParams strata-client: fullnode bug fix, ws to http strata-client: rename DepositTxParams to Config resolve comments
voidash
force-pushed
the
STR-478-FULL-NODE
branch
from
14164e3
to
54e409a
Compare
| .build(url) | ||
| .await | ||
| .expect("Failed to connect to the RPC server") | ||
| } |
There was a problem hiding this comment.
@voidash Can you make another PR with just changes related to this fix without the other param changes ?
There was a problem hiding this comment.
This should be named something like fn_sync_flaky_peers. The point of the test isn't that we're testing full node sync, we're trying to that we can keep syncing as connections are interrupted. The first component of the test name is supposed to be some term describing what function it pertains to. It's also good to add a comment at the start of the file that describes what the test does.
It might actually make sense to split it into 2 or 3 tests that test the different parties restarting.
| assert ( | ||
| net_balance == deposit_amount | ||
| ), f"deposit processed multiple times, extra: {balance - original_balance - deposit_amount}" | ||
| assert balance == deposit_amt * SATS_TO_WEI, f"deposit processed multiple times: {balance}" |
There was a problem hiding this comment.
This kinda seems like it's duplicating the above checks? Should specify that that was the amount accepted and what the expected amount was.
Description
Type of Change
Checklist
Related Issues