Improvement RGB Isa instructions

[crisdut]

Description

This PR intent:

• Add new macro instructions: cng and cnc.
• Add support for use lds and ldg with registers [1].
• Fix decode for cnp, cns, cng and cnc

[1] Depends:

• https://github.com/AluVM/rust-aluvm/pull/100

[crisdut]

Very good work!

I think we need to get rid of old commands - I do not see the usecase for the fixed index. If someone needs one, it is just two instructions, one to load the fixed index into a register.

Today, only the UDA uses these two instructions with fixed indexes. I can change the code after rgb-core new release.

Can you pls re-work PR by not adding new operations and instead modifying existing ones?

Yes, I will make that!

[dr-orlovsky]

Today, only the UDA uses these two instructions with fixed indexes. I can change the code after rgb-core new release.

Yes. But I will update the UDA script then

[crisdut]

Today, only the UDA uses these two instructions with fixed indexes. I can change the code after rgb-core new release.

Yes. But I will update the UDA script then

Ok, sure. I reviewed the UDA script and I noticed the ldp uses a fixed index too. I think it would be better to change the ldc and ldp to support register instead of fixed indexes, right?

[dr-orlovsky]

Yes, correct. We than can push constant number to a register and just add one instruction to the script in front of ldc and ldp

[crisdut]

Hi @dr-orlovsky,

I was thinking, ldc and ldg are equivalent operations, right?

Is this separation due to operations such as Genesis, Transition and Enxtension?

[dr-orlovsky]

They are not. ldc takes the contract global state, while ldg takes operations's global state (which, with the only except of genesis operation is not ever equal to contract).

[crisdut]

They are not. ldc takes the contract global state, while ldg takes operations's global state (which, with the only except of genesis operation is not ever equal to contract).

Thanks for reply!

BTW, I finish the changes!

[dr-orlovsky]

Can you pls re-do this PR against master and not v0.11 branch so we have CI running?

Also can you please run cargo +nightly fmt --all to remove the formatting changes which complicate the review?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 78 lines in your changes are missing coverage. Please review.

Project coverage is 15.5%. Comparing base (0716f0e) to head (010603f). Report is 3 commits behind head on master.

Files	Patch %	Lines
src/vm/op_contract.rs	0.0%	78 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #201 +/- ## ====================================== Coverage 15.5% 15.5% ====================================== Files 33 33 Lines 4165 4158 -7 ====================================== Hits 644 644 + Misses 3521 3514 -7 ``` | [Flag](https://app.codecov.io/gh/RGB-WG/rgb-core/pull/201/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG) | Coverage Δ | | |---|---|---| | [rust](https://app.codecov.io/gh/RGB-WG/rgb-core/pull/201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG) | `15.5% <0.0%> (+<0.1%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Gigi3d]

amazing stuff

[dr-orlovsky]

Adding a global contract state seems to be very challenging and will require refactoring validation workflow: https://github.com/RGB-WG/rgb-core/pull/208

I propose to restrict this PR to the changes in already implemented instructions - and leave LdG for v0.12

[crisdut]

Adding a global contract state seems to be very challenging and will require refactoring validation workflow: #208

Great!

I propose to restrict this PR to the changes in already implemented instructions - and leave LdG for v0.12

I think you mean LdC, because in the master branch this instruction is not implemented yet. Right?

[dr-orlovsky]

I think you mean LdC, because in the master branch this instruction is not implemented yet. Right?

Yes

[crisdut]

There were multiple issues so I decided to proof them by myself. Pls check the last three commitments

Sorry Maxim, I dont understand why they are issues, as I was using these registers from the beginning. Could you explain me?

[dr-orlovsky]

List of things I have changed:

• src_reg was not modified, while the commands now accept a source regisger
• the arguments can now fit byte if we shorten reg no to 16-bit. This also changes the size of the commands
• LdF was not changed
• docs had to be updated
• assembly impl for ldp contained a error

[dr-orlovsky]

Most of these things I proved in https://github.com/RGB-WG/rgb-core/pull/201/commits/8752e06f38321019c3256544f8b570dca302ce84, but in other commitments as well

[crisdut]

Most of these things I proved in 8752e06, but in other commitments as well

Maxim, I think I found other issues, I will publish new commit.

Can you review, please?

[dr-orlovsky]

Sure!

[dr-orlovsky]

@crisdut why do you think we need this? Reg32, Reg16 and Reg8 are just different bit-length for the registry index, it has nothing to do with a8/a16/a32... The proposed commit breaks word alignment and adds "dummy" bits, which is unnecessary

[crisdut]

@crisdut why do you think we need this? Reg32, Reg16 and Reg8 are just different bit-length for the registry index, it has nothing to do with a8/a16/a32... The proposed commit breaks word alignment and adds "dummy" bits, which is unnecessary

Oh Sorry, I understood that it was necessary to make these changes to follow with change in registers, but it really makes sense to avoid these changes.

I force-push it to remove it.

[dr-orlovsky]

Tank you! I know this thing is confusing - I even did the same myself during work on this PR and then recalled that a16 doesn't imply Reg16 :)

Probably I have to rename types in AluVM...

[crisdut]

LGTM, but I need your approval on my changes, that I do not miss something - and then I will merge

I tested here and the code works.

ACK.

[crisdut]

Hi @zoedberg @nicbus

Do you need any help getting this implementation started, or can we do the merge?

[zoedberg]

@crisdut could you please rebase this on top of master? otherwise it doesn't compile in rgb-lib

[crisdut]

@crisdut could you please rebase this on top of master? otherwise it doesn't compile in rgb-lib

This is very strange, could you add here the reason for the problem.

It is worth remembering that as we changed the AluVM instructions, we also need to update the rgb-schemata as well, as all instructions that used fixed indexes now use registers.

[crisdut]

It is worth remembering that as we changed the AluVM instructions, we also need to update the rgb-schemata as well, as all instructions that used fixed indexes now use registers.

If you prefer, I can open the PR containing the changes and then you can test it with the two updated crates. What do you think?

[dr-orlovsky]

@crisdut I think the reason that we break the AluVM compatibility, this first we need to update schemata with a different code

[crisdut]

@crisdut I think the reason that we break the AluVM compatibility, this first we need to update schemata with a different code

Yes, I believe that's exactly what it is.

But we need to not only change the rgb-schemata, but also the rgb-std, as the size of the inputs in the operations was changed from tiny to small (see here), therefore, the transitions in the rgb-std need to be modified (but specifically, here). Right?

[crisdut]

BTW,

@zoedberg I created two draft PRs, please see:

• https://github.com/RGB-WG/rgb-schemata/pull/31
• https://github.com/RGB-WG/rgb-std/pull/152

Could you check if the problem persists with these PRs?

If yes, please post the error log in the thread.

Hope this helps

[dr-orlovsky]

Right?

Correct

[zoedberg]

@crisdut I've patched the 2 PRs and now it compiles. I've ran rgb-lib's test suite and everything works good so I think we can merge your PRs.

[Gigi3d]

https://giphy.com/gifs/travisband-MVJSzagt5zYMqEUAmY