feat: enable DAP-backed transaction script debugging#1959
feat: enable DAP-backed transaction script debugging#1959igamigo merged 10 commits into0xMiden:mainfrom
Conversation
|
Original PR was #1883 (but the base branch was deleted) cc @juan518munoz @bitwalker |
|
We need to publish new crates to |
| pub async fn prepare_offline_bootstrap(&mut self) -> Result<(), ClientError> { | ||
| let limits = self.store.get_rpc_limits().await?.unwrap_or_default(); | ||
| self.store.set_rpc_limits(limits).await?; | ||
| self.rpc_api.set_rpc_limits(limits).await; | ||
|
|
||
| if let Some((genesis, _)) = self.store.get_block_header_by_num(BlockNumber::GENESIS).await? | ||
| { | ||
| self.rpc_api.set_genesis_commitment(genesis.commitment()).await?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let genesis = synthetic_offline_genesis_header(); | ||
| let blank_mmr_peaks = MmrPeaks::new(Forest::empty(), vec![]) | ||
| .expect("Blank MmrPeaks should not fail to instantiate"); | ||
| self.store.insert_block_header(&genesis, blank_mmr_peaks, false).await?; | ||
| self.rpc_api.set_genesis_commitment(genesis.commitment()).await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
In the current state, offline seems like a purely testing feature, is this correct?
For example, what happens if I create a new account in the offline mode but then want to use this same client for other accounts? (IIUC, we will have written a synthetic genesis block commitment to the store, which will be incompatible once we connect to a real node)
There was a problem hiding this comment.
Yes, this is purely for local testing/debugging — it lets you create an account and execute programs without needing a running node. The synthetic genesis header is only meant to satisfy the client's requirement that a genesis block exists in the store.
If you then connect to a real node and sync, the genesis commitment mismatch would indeed be a problem. The expectation is that you'd use a fresh store (separate --store-path or rm the DB) when switching between offline and network modes. We could add a guard or warning for this in the future.
There was a problem hiding this comment.
The expectation is that you'd use a fresh store
This seems a bit fragile. It seems like a feature that should only be available in testing/debug mode. Are there any alternatives to adding an offline argument on this command?
There was a problem hiding this comment.
Good point. We could gate the --offline flag behind a cfg(feature = "testing"). Alternatively, we could skip the CLI flag entirely and only expose prepare_offline_bootstrap() as a library method for test harnesses — the DAP debugging workflow in the CLI could call it internally when --start-debug-adapter is used without an active node connection, rather than requiring the user to explicitly pass --offline at account creation time.
8f5fa90 to
1c2072c
Compare
|
Btw, we still need new |
1c2072c to
e9c2506
Compare
|
@juan518munoz I updated dependencies to use the newest crates from crates.io |
|
Hi @djolertrk! Client |
| /// | ||
| /// If this binary was built without DAP support, using this flag returns an error. | ||
| #[arg(long = "start-debug-adapter")] | ||
| start_debug_adapter: Option<String>, |
There was a problem hiding this comment.
I think we can use SocketAddr here, right?
There was a problem hiding this comment.
Yes, good call. Will switch to SocketAddr — clap can parse it directly.
| /// This stores default RPC limits and inserts a synthetic genesis header if one is not | ||
| /// already present in the store. The synthetic header is only intended for local-only | ||
| /// execution and debugging. | ||
| pub async fn prepare_offline_bootstrap(&mut self) -> Result<(), ClientError> { |
There was a problem hiding this comment.
It feels like this is emulating some of what the MockRpcApi already does. Basically, you should be able to start a client with this RPC component (instead of the actual gRPC client), and you get a mockchain that you can manage yourself. This makes it so you don't have to manually mock objects - things like the genesis header will come automatically by calling client.sync_state() as long as the mockchain has been initialized and has at least 1 block.
There was a problem hiding this comment.
Good suggestion. As I proposed above, for now, I've gated --offline and prepare_offline_bootstrap behind #[cfg(feature = "testing")] so it's not available in production builds. Switching to MockRpcApi would require restructuring how the CLI constructs the client (the RPC backend is set before commands run), so I'd prefer to do that as follow-up. Filed as a TODO.
| must_use_candidate = "allow" # This marks many fn's which isn't helpful. | ||
| should_panic_without_expect = "allow" # We don't care about the specific panic message. | ||
| # End of pedantic lints. | ||
|
|
| async fn execute_program<AUTH: Keystore + Sync + 'static>( | ||
| &self, | ||
| client: &mut Client<AUTH>, | ||
| account_id: AccountId, | ||
| tx_script: TransactionScript, | ||
| advice_inputs: AdviceInputs, | ||
| ) -> Result<[Felt; 16], CliError> { |
There was a problem hiding this comment.
Will this compile well without the dap feature? Seems like it uses execute_program_with_dap
Good point, I will retarget this to |
e9c2506 to
dff01aa
Compare
aee77f5 to
43a8bd8
Compare
|
cc @juan518munoz - it is using crates from crates.io now. This is the last piece to land for enabling the debugging of real transactions with the |
mmagician
left a comment
There was a problem hiding this comment.
Looks good, could you further document how to use this please? (including the two feature gates and what they do)
Thanks! |
It would probably be good if it lived alongside all the other documentation we have (see how |
@mmagician addressed. Thanks! |
|
ping :) |
There was a problem hiding this comment.
LGTM! I left some more small comments in order to see if we can simplify code and minimize branching/cfg conditionals. Would be nice if @mmagician (or anyone else) could take another look.
|
Thanks a lot for your comments! Can someone merge this? |
| let output_stack = | ||
| self.execute_program(&mut client, account_id, tx_script, advice_inputs).await?; | ||
|
|
||
| match result { | ||
| Ok(output_stack) => { | ||
| println!("Program executed successfully"); | ||
| println!("Output stack:"); | ||
| self.print_stack(output_stack); | ||
| Ok(()) | ||
| }, | ||
| Err(err) => Err(CliError::Exec(err.into(), "error executing the program".to_string())), | ||
| } | ||
| println!("Program executed successfully"); | ||
| println!("Output stack:"); | ||
| self.print_stack(output_stack); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Is this change proposed by make lint? Otherwise we could undo it so we keep the diff as short as possible.
There was a problem hiding this comment.
Good point. This was not needed for lint.
There was a problem hiding this comment.
This file has many std imports that could be top-leveled.
|
|
||
| if config_handle.restart_requested() { | ||
| config_handle.reset_restart(); | ||
| eprintln!("Recompiling from source and restarting debug session..."); |
There was a problem hiding this comment.
Can this be considered an error?
| eprintln!("Recompiling from source and restarting debug session..."); | |
| println!("Recompiling from source and restarting debug session..."); |
| "`--offline` cannot be combined with `--deploy`".to_string(), | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can't we use claps conflict_with? For example
#[arg(long, default_value_t = false, conflicts_with = "init_storage_data_path")]| use miden_protocol::account::AccountId; | ||
| use miden_protocol::block::account_tree::AccountTree; | ||
| use miden_protocol::block::nullifier_tree::NullifierTree; | ||
| use miden_protocol::block::{BlockNoteTree, Blockchain, FeeParameters}; | ||
| use miden_protocol::crypto::dsa::ecdsa_k256_keccak::SecretKey; | ||
| use miden_protocol::crypto::merkle::smt::Smt; | ||
| use miden_protocol::transaction::{OrderedTransactionHeaders, TransactionKernel}; |
There was a problem hiding this comment.
let's move this upwards
| validator_key, | ||
| fee_parameters, | ||
| 0, | ||
| ) |
There was a problem hiding this comment.
Is there any reason as to not using BlockHeader::mock(BlockNumber::GENESIS, None, None, &[], TransactionKernel.to_commitment())?
There was a problem hiding this comment.
No, using BlockHeader::mock(...) is cleaner here. Updated to use it.
|
|
||
| ```bash | ||
| # Default build (DAP enabled) | ||
| cargo build -p miden-client-cli |
There was a problem hiding this comment.
Is there any significant overhead building with dap enabled? If so it'd be preferable to leave it as a non-default feature.
There was a problem hiding this comment.
I’d prefer to keep it enabled by default. The DAP support is just pulled from miden-debug, and it does not affect normal execution unless the user explicitly passes --start-debug-adapter. Keeping it default makes this important debugging path available out of the box, while users who want a smaller build can still opt out with --no-default-features.
There was a problem hiding this comment.
I think it makes sense for it to be a default feature, since as @djolertrk pointed out, it will mean that clients are able to be used for debugging out of the box. But so long as the client installed by midenup has the feature enabled, then it isn't as important whether the feature is actually in the default set or not (though I think it would be important to document for users building from source that the feature is needed to support debugging).
There was a problem hiding this comment.
I have added one more sentence into the docs to clarify this. Thanks @bitwalker!
| let limits = self.store.get_rpc_limits().await?.unwrap_or_default(); | ||
| self.store.set_rpc_limits(limits).await?; | ||
| self.rpc_api.set_rpc_limits(limits).await; |
There was a problem hiding this comment.
Maybe this can be replaced with a call to ensure_rpc_limits_in_place
There was a problem hiding this comment.
Not directly. ensure_rpc_limits_in_place() fetches limits from the node if they are not already cached, while prepare_offline_bootstrap() must work without a node. Here we intentionally seed limits from the store or RpcLimits::default() and then cache them in both the store and RPC client.
Add support for interactive debugging of transaction scripts via the Debug Adapter Protocol (DAP). This includes: - `--start-debug-adapter` flag on the `exec` CLI command - `execute_program_with_dap` method in the client - Offline bootstrap mode (`--offline` flag) for creating accounts and executing programs without a node connection - Optional `dap` feature gate on `miden-client` and `miden-client-cli`
…efault - Use miden-vm v0.22.1 and protocol v0.14.3 from crates.io - Enable DAP feature by default in rust-client and CLI - Update Package construction for v0.22.1 API
Replace local path dependency with published crate.
- Use SocketAddr instead of String for --start-debug-adapter flag - Gate --start-debug-adapter behind #[cfg(feature = "dap")] so it only appears when the feature is enabled - Remove trailing empty line in workspace Cargo.toml
5ed3667 to
b2add88
Compare
|
someone needs to approve the CI. :) |
|
@igamigo @juan518munoz Looks to me like this is ready to merge - are there any remaining obstacles? |
I was hoping to get one final pass from @mmagician (pinged him separately and he told me he'd take a look), but otherwise we should be good to proceed. |
|
ping :) |
|
Merging this for now. If @mmagician or anyone has any further comments, we can open an issue to discuss. |
Add support for interactive debugging of transaction scripts via the Debug Adapter Protocol (DAP).
This includes:
--start-debug-adapterflag on theexecCLI commandexecute_program_with_dapmethod in the client--offlineflag) for creating accounts and executing programs without a node connectiondapfeature gate onmiden-clientandmiden-client-cli