feat: enable TX debugging#1883
feat: enable TX debugging#1883djolertrk wants to merge 14 commits into0xMiden:release/v0.14.0-betafrom
Conversation
e502fbb to
ec3fa67
Compare
|
Rebased on top of |
| [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
There was a problem hiding this comment.
Let's update the branch so we can run the CI
| 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.
|
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. |
c310c9c to
4a3fbf6
Compare
|
hey @juan518munoz, the PR in |
|
cc @bitwalker |
juan518munoz
left a comment
There was a problem hiding this comment.
Thanks for updating the PR! Looks like it won't be hard to merge as there is no breaking change, but there are still some comments that need to be addressed.
| path = "src/main.rs" | ||
|
|
||
| [features] | ||
| # Keep debugger integration optional while `miden-debug` requires a newer Rust version than the |
There was a problem hiding this comment.
I'm not sure by this is mandatory, why does one require a higher version than the other?
There was a problem hiding this comment.
It is not the case anymore, the compiler is the same. Will remove the stale comment.
| should_panic_without_expect = "allow" # We don't care about the specific panic message. | ||
| # End of pedantic lints. | ||
|
|
||
| [patch.crates-io] |
There was a problem hiding this comment.
This patches need to be removed before merging
There was a problem hiding this comment.
In order to do it, we will need to wait for this to be published to creates.io
| miden-utils-diagnostics = { git = "https://github.com/0xMiden/miden-vm.git", rev = "1b60fa4b54e60075cad7a761b593d5c00c6aaf46" } | ||
| miden-utils-indexing = { git = "https://github.com/0xMiden/miden-vm.git", rev = "1b60fa4b54e60075cad7a761b593d5c00c6aaf46" } | ||
|
|
||
| [patch."https://github.com/0xMiden/miden-base"] |
Thanks a lot for the comments! |
74204f3 to
c5b6c4a
Compare
| /// Executes the provided transaction script with a DAP debug adapter listening for | ||
| /// connections, allowing interactive debugging via any DAP-compatible client. | ||
| #[cfg(feature = "dap")] | ||
| pub async fn execute_program_with_dap( |
There was a problem hiding this comment.
This function is almost to execute_program, can't we generalize it?
| } | ||
| } | ||
|
|
||
| fn synthetic_offline_genesis_header() -> BlockHeader { |
There was a problem hiding this comment.
Correct me if wrong, but this functions seems redundant as we can achieve the same with MockChainBuilder
MockChainBuilder::new()
.native_asset_id(some_id)
.verification_base_fee(500)
.build()
.expect("mock chain build should be valid")
.genesis_block_header()There was a problem hiding this comment.
Hmm, but we would need to mark the miden-testing a non-optional dependency
| } | ||
|
|
||
| #[test] | ||
| fn synthetic_offline_genesis_header_matches_empty_chain_state() { |
There was a problem hiding this comment.
If the other comment applies, we should remove this.
Use miden-debug's DapExecutorFactory and DapConfig to let miden-client execute transaction scripts under a debug adapter via --start-debug-adapter.
c5b6c4a to
f5ed3a7
Compare
|
Why did we delete the Btw, we are waiting for the crates for |

Enable transaction debugging with Debugger Adapter Protocol.