Conversation
djolertrk
force-pushed
the
pr/enable-tx-debugging-clean
branch
from
e502fbb to
ec3fa67
Compare
|
Rebased on top of |
Cargo.toml
Outdated
| [patch.crates-io] | ||
| miden-protocol = { path = "../protocol/crates/miden-protocol" } | ||
| miden-standards = { path = "../protocol/crates/miden-standards" } | ||
| miden-testing = { path = "../protocol/crates/miden-testing" } | ||
| miden-tx = { path = "../protocol/crates/miden-tx" } |
There was a problem hiding this comment.
This needs to be removed
| miden-node-ntx-builder = { version = "=0.14.0-alpha.4" } | ||
| miden-node-proto = { version = "=0.14.0-alpha.4" } | ||
| miden-node-proto-build = { default-features = false, version = "=0.14.0-alpha.4" } | ||
| miden-node-proto-build = { default-features = false, path = "../node/proto", version = "=0.14.0-alpha.1" } |
There was a problem hiding this comment.
This also needs to be fixed.
There was a problem hiding this comment.
Yes, sure. I should have marked this as a draft.
|
Could you provide a more detailed description like screenshots and a sample debug flow? |
Hi @juan518munoz, thanks for taking a look. The main idea is to enhance debugger with support to debug programs executed under the transaction kernel, instead of only debugging standalone VM programs. Please check: 0xMiden/miden-debug#41 Within 0xMiden/miden-debug#29, I have described one of possible use cases, but I was able to add even an "offline" mode to this dap execution, so you just do: run # terminal 1
$ miden-client init --local --network localhost
$ miden-client new-wallet --offline
$ miden-client exec \
--script-path /tmp/miden-no-node-client/nop.masm \
--start-debug-adapter localhost:4711and run debugger as: # terminal 2
miden-debug --dap-connect localhost:4711so, it starts the debugger in TUI mode, and you can step through the execution, and whenever you execute a command via DAP, it will be logged with the client -- see picture attached. 
|
juan518munoz
left a comment
juan518munoz
left a comment
There was a problem hiding this comment.
Let's update the branch so we can run the CI
bin/miden-cli/src/commands/exec.rs
Outdated
| client | ||
| .execute_program(account_id, tx_script, advice_inputs, BTreeSet::new()) | ||
| .await | ||
| }; | ||
|
|
||
| #[cfg(not(feature = "dap"))] | ||
| let result = client | ||
| .execute_program(account_id, tx_script, advice_inputs, BTreeSet::new()) | ||
| .await; |
There was a problem hiding this comment.
This is duplicated code. Maybe the best approach is not to split between feature = "dap" or not.
| path = "src/main.rs" | ||
|
|
||
| [features] | ||
| dap = ["miden-client/dap", "dep:miden-debug"] |
There was a problem hiding this comment.
Should we gate behind a feature? Or have connection depend on an Optional argument?
There was a problem hiding this comment.
I moved the CLI behavior toward the optional-argument model. The --start-debug-adapter is now always part of the CLI surface, and if the binary is built without DAP support it returns a clear runtime error. Furthermore, I kept the actual debugger wiring behind the dap feature because miden-debug currently requires a newer Rust version than the rest of the client workspace. So making it unconditional would raise the default build/toolchain requirements for miden-client. If that constraint goes away, we can simplify further and drop the feature gate.
| use crate::store::{BlockRelevance, StoreError}; | ||
| use crate::{Client, ClientError}; | ||
|
|
||
| const OFFLINE_NATIVE_ASSET_FAUCET_ID: u128 = 0xab00_0000_0000_cd20_0000_ac00_0000_de00; |
There was a problem hiding this comment.
The whole diff of this part seems to be caused due to not being on track with next. As we should already have solved them here.
Use miden-debug's DapExecutorFactory and DapConfig to let miden-client execute transaction scripts under a debug adapter via --start-debug-adapter.
|
djolertrk
force-pushed
the
pr/enable-tx-debugging-clean
branch
from
f33d10b to
5c75028
Compare
|
@djolertrk I've enabled CI runs on this PR. Let's wait for 0xMiden/miden-debug#41 as you said and update to it. Once it's done and the CI passes feel free to re-request reviews so we can merge this. |
2 participants
This was rebased on top of Fumuran's branch for migrating to
miden-vm v0.21.cc @bitwalker