Add the test that executes counter contract, basic wallet and p2id note script on the local node

[greenhat]

Close #554
Ref #544

This PR:

• Introduces the miden-integration-node-tests crate with a counter contract test (complete) and basic wallet and p2id note tests running against a local node; the tests are using the same node instance with the first test starting the node and the last one killing it; the local node is bootstrapped on each run;
• Fixes for the expected Word elements order on the stack in the Miden SDK MASM procedures;
• Removes Scenario testing harness for the testnet;

[greenhat]

@bitwalker I've tried to use Scenario type for the counter_contract test on the testnet, and I stumbled on one lacking feature. When I want to get the note ID when I'm creating the output note so that I can use it later when consuming the note. I haven't figured out an ergonomic way to implement it with its builder-like API. So I temporarily wrote a new test using the Miden client directly.

Besides that, I've been thinking about the target audience for the testing harness, and I think it might be too opinionated for the end users. Meaning that they would have to familiarize themselves with Miden client API anyway when they will be writing their off-chain code. And now we are forcing them to also learn an opinionated framework to write their tests instead of just letting them use the already familiar Miden client API that they've used in their off-chain code.

[bitwalker]

@greenhat To be clear, the testing harness in the integration test suite is for internal use only, the public test harness would almost certainly look different, and likely incorporate the efforts by the node and client teams in terms of describing test setup/scenarios.

If the Scenario type isn't achieving the goal of allowing us to declaratively express test scenarios at a high level, while hiding the boilerplate/details of the underlying client orchestration, then it isn't suited to its purpose, and I think it should be rewritten/refactored so that it does. I still think writing tests directly against the miden-client APIs in our integration test suite is far too low-level at this point in time, and is going to cause a lot of churn as that API evolves. Ideally we could keep that churn contained in a single place, more or less, but it really depends on if there are repeated patterns in the tests.

[greenhat]

@bobbinth @bitwalker @partylikeits1983
I dug into the order of the word elements on the stack in the stdlib and miden-base API. It turned out that the order of the felts on the stack when passing a word is consistent across the limited bindings we have for the standard library API and miden-base API. The word is expected with the least significant felt on top of the stack, i.e., [f0, f1, f2, f3] word is expected to be as [f3, f2, f1, f0] on the stack. For some reason I assumed the opposite.

However, I found that hash_1to1 expects the 8 4-byte parts(felts) in the order with the most significant bytes on top of the stack. So it seems, when it comes to multiple felts that are not words the order expected on the stack is different.

In the upcoming type signatures and automated Rust bindings generation we need to differentiate words from multiple felts and encode the words in reverse order in the Rust bindings.

[bobbinth]

However, I found that hash_1to1 expects the 8 4-byte parts(felts) in the order with the most significant bytes on top of the stack. So it seems, when it comes to multiple felts that are not words the order expected on the stack is different.

I'm assuming you mean SHA, Blake, and Keccak hashing procedures here, right? If so, this was done originally and never fixed. We actually have an issue for this (0xMiden/miden-vm#571) but it's been open quite some time ago.

We could change the order of inputs for these procedures and the internally just reverse them again. This would avoid rewriting the entire procedures at the expense of 10 - 15 cycles (I think).

[greenhat]

@bitwalker This PR has been going on for quite some time. I want to do the remaining part of the basic-wallet test (note creation) in a new PR after I do the tx script compilation. I cleaned up and polished this PR, and it's ready for review.

[greenhat]

I tried to clean up the commits, but since there was a lot of back and forth on some features, it's still a mess. We can merge it via the squash and merge.

[bitwalker]

Maybe we should move shared test code like that into a midenc-testing crate, and then share it between miden-integration-tests (which we should probably rename to `midenc-integration-tests) and this crate?

Having this crate depend on miden-integration-tests sort of defeats the point of splitting it out into its own crate IMO.

[greenhat]

Good point! I agree.

[bitwalker]

Let's name this midenc-integration-network-tests, since the tests aren't really testing the node (which the name kind of implies), instead we're testing programs against a local version of the network (i.e. testnet)

We should use the midenc- prefix for any compiler crates, even if they aren't published, since the miden- prefix is heavily used by other parts of the project, and it helps us to avoid naming conflicts, and provides a bit more context for where the crate fits. Ideally we'd namespace all Miden crates better than we do now, but that ship has mostly already sailed, the compiler is really the only one that has been consistent about using its own prefix.

[greenhat]

Let's name this midenc-integration-network-tests, since the tests aren't really testing the node (which the name kind of implies), instead we're testing programs against a local version of the network (i.e. testnet)

I agree the name is a bit misleading. When I see the network in the name I'm thinking of the tests that are relying on the external services. Also, testnet means for me the "official" instance of the chain running by the creators for the testing purposes. So when I bootstrap the local node in my test I'm creating the new instance of the chain on every test run. For some reason "local node" describes it for me the best. How about midenc-integration-local-chain-tests?

[bitwalker]

Looks good overall! I didn't do a deep dive into the node setup code, but from a quick skim I didn't see anything that seemed like it needs to be addressed in this PR. At some point I'd like to walk through how it works with you so that I have a better understanding of it, but the broad strokes make sense.

I'm marking this approved, but have a couple of nits I'd like to address. I'll leave it up to you whether it is easier to do them in this PR, or open a follow-on PR that makes them. Your call.

[greenhat]

I'd like to merge it as is. I made #629 to address the comments in the next PR.