Improvement RGB Isa instructions #201

crisdut · 2024-01-15T18:20:49Z

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:

Fix cnv macro (and improviment asm_macro) AluVM/aluvm#100

dr-orlovsky

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.

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

crisdut · 2024-01-19T10:37:04Z

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 · 2024-01-19T10:51:53Z

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 · 2024-01-19T11:03:09Z

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 · 2024-01-19T11:23:06Z

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 · 2024-01-20T15:45:06Z

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 · 2024-01-20T15:55:03Z

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 · 2024-01-20T16:10:39Z

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 · 2024-01-20T18:33:13Z

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 · 2024-01-21T09:49:04Z

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 ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #201   +/-   ##
======================================
  Coverage    15.5%   15.5%           
======================================
  Files          33      33           
  Lines        4165    4158    -7     
======================================
  Hits          644     644           
+ Misses       3521    3514    -7

Flag	Coverage Δ
rust	`15.5% <0.0%> (+<0.1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dr-orlovsky

I found several logical mistakes i the past and new code, which we have to address

src/vm/macroasm.rs

src/vm/op_contract.rs

dr-orlovsky

Pls see my comment

src/vm/op_contract.rs

Gigi3d · 2024-02-13T03:23:31Z

amazing stuff

dr-orlovsky · 2024-02-18T21:50:14Z

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

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

crisdut · 2024-02-26T12:25:06Z

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 · 2024-02-26T16:02:37Z

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

Yes

dr-orlovsky · 2024-02-27T19:55:25Z

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 · 2024-02-27T19:56:13Z

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

crisdut · 2024-02-27T20:52:52Z

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 · 2024-02-27T21:11:27Z

Sure!

dr-orlovsky · 2024-02-27T21:23:02Z

@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 · 2024-02-27T21:38:13Z

@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 · 2024-02-27T21:43:20Z

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...

dr-orlovsky

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

crisdut · 2024-02-28T03:11:52Z

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.

src/vm/macroasm.rs

dr-orlovsky

ACK 010603f

crisdut · 2024-02-29T16:30:03Z

Hi @zoedberg @nicbus

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

zoedberg · 2024-02-29T17:10:45Z

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

crisdut · 2024-02-29T17:18:17Z

@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 · 2024-02-29T17:21:28Z

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 · 2024-03-01T01:47:30Z

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

crisdut · 2024-03-01T10:38:19Z

@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 · 2024-03-01T12:28:14Z

BTW,

@zoedberg I created two draft PRs, please see:

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 · 2024-03-01T13:09:06Z

Right?

Correct

zoedberg · 2024-03-01T13:57:34Z

@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.

dr-orlovsky

ACK 010603f

Gigi3d · 2024-03-06T03:18:12Z

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

crisdut requested a review from dr-orlovsky January 15, 2024 18:21

crisdut force-pushed the feat/udc branch from 0d3e910 to 4abad5e Compare January 15, 2024 18:21

crisdut mentioned this pull request Jan 15, 2024

Support to Collections ("unique digital collection" or UDC) RGB-WG/rgb-schemata#24

Draft

6 tasks

dr-orlovsky reviewed Jan 18, 2024

View reviewed changes

crisdut force-pushed the feat/udc branch from 4abad5e to bc30841 Compare January 20, 2024 16:10

crisdut changed the base branch from v0.11 to master January 21, 2024 08:33

crisdut force-pushed the feat/udc branch from bc30841 to 50afbbe Compare January 21, 2024 09:45

crisdut requested a review from dr-orlovsky January 21, 2024 09:58

dr-orlovsky requested changes Jan 21, 2024

View reviewed changes

src/vm/macroasm.rs Outdated Show resolved Hide resolved

src/vm/op_contract.rs Show resolved Hide resolved

src/vm/op_contract.rs Outdated Show resolved Hide resolved

src/vm/op_contract.rs Show resolved Hide resolved

src/vm/op_contract.rs Outdated Show resolved Hide resolved

crisdut force-pushed the feat/udc branch from 50afbbe to 73d7b0c Compare January 22, 2024 14:48

crisdut requested a review from dr-orlovsky January 22, 2024 14:49

dr-orlovsky requested changes Jan 26, 2024

View reviewed changes

src/vm/op_contract.rs Outdated Show resolved Hide resolved

dr-orlovsky mentioned this pull request Feb 18, 2024

Add contract global state in VM #208

Closed

dr-orlovsky added this to the v0.11.0 milestone Feb 26, 2024

dr-orlovsky assigned crisdut Feb 27, 2024

crisdut force-pushed the feat/udc branch from 73d7b0c to f1af65d Compare February 27, 2024 16:51

feat: update rgb isa instrs

d8269f3

dr-orlovsky added the *security* Issues affecting safety/security (include undefined behaviours) label Feb 27, 2024

crisdut force-pushed the feat/udc branch from 7b8c675 to 1ee6738 Compare February 27, 2024 21:39

crisdut requested a review from dr-orlovsky February 27, 2024 21:41

dr-orlovsky previously approved these changes Feb 27, 2024

View reviewed changes

crisdut commented Feb 28, 2024

View reviewed changes

src/vm/macroasm.rs Outdated Show resolved Hide resolved

fix: imports

010603f

crisdut dismissed dr-orlovsky’s stale review via 010603f February 28, 2024 15:21

dr-orlovsky approved these changes Feb 28, 2024

View reviewed changes

This was referenced Mar 1, 2024

Fix collection size convertion RGB-WG/rgb-std#152

Merged

Update contracts and AluScript RGB-WG/rgb-schemata#31

Merged

dr-orlovsky approved these changes Mar 1, 2024

View reviewed changes

dr-orlovsky merged commit 916ff4f into RGB-WG:master Mar 1, 2024
18 of 21 checks passed

Improvement RGB Isa instructions #201

Improvement RGB Isa instructions #201

Conversation

crisdut commented Jan 15, 2024

dr-orlovsky left a comment

Choose a reason for hiding this comment

crisdut commented Jan 19, 2024

dr-orlovsky commented Jan 19, 2024

crisdut commented Jan 19, 2024

dr-orlovsky commented Jan 19, 2024 • edited Loading

crisdut commented Jan 20, 2024

dr-orlovsky commented Jan 20, 2024

crisdut commented Jan 20, 2024

dr-orlovsky commented Jan 20, 2024

codecov bot commented Jan 21, 2024 • edited Loading

Codecov Report

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

Gigi3d commented Feb 13, 2024

dr-orlovsky commented Feb 18, 2024 • edited Loading

crisdut commented Feb 26, 2024

dr-orlovsky commented Feb 26, 2024

dr-orlovsky commented Feb 27, 2024

dr-orlovsky commented Feb 27, 2024

crisdut commented Feb 27, 2024

dr-orlovsky commented Feb 27, 2024

dr-orlovsky commented Feb 27, 2024 • edited Loading

crisdut commented Feb 27, 2024 • edited Loading

dr-orlovsky commented Feb 27, 2024

dr-orlovsky left a comment

Choose a reason for hiding this comment

crisdut commented Feb 28, 2024

dr-orlovsky left a comment

Choose a reason for hiding this comment

crisdut commented Feb 29, 2024 • edited Loading

zoedberg commented Feb 29, 2024

crisdut commented Feb 29, 2024

crisdut commented Feb 29, 2024

dr-orlovsky commented Mar 1, 2024

crisdut commented Mar 1, 2024 • edited Loading

crisdut commented Mar 1, 2024

dr-orlovsky commented Mar 1, 2024

zoedberg commented Mar 1, 2024

dr-orlovsky left a comment

Choose a reason for hiding this comment

Gigi3d commented Mar 6, 2024

dr-orlovsky commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 21, 2024 •

edited

Loading

dr-orlovsky commented Feb 18, 2024 •

edited

Loading

dr-orlovsky commented Feb 27, 2024 •

edited

Loading

crisdut commented Feb 27, 2024 •

edited

Loading

crisdut commented Feb 29, 2024 •

edited

Loading

crisdut commented Mar 1, 2024 •

edited

Loading