Include size lookup of contract for computing LDC opcode cost #598

MitchTurner · 2023-10-03T19:13:10Z

fuel-vm/src/interpreter/blockchain.rs

fuel-vm/src/interpreter/executors/instruction.rs

fuel-vm/src/tests/blockchain.rs

MitchTurner · 2023-10-06T01:27:32Z

It looks like the storage allows getting the contract bytes for a specified buffer, so we only copy what is specified with $rC now, not the entire contract :).

I've added a test that copies a contract with a bunch of noops and checks the costs based on that.

Still might clean up these tests a bit. There is some DRYing I can do before merging this.

fuel-vm/src/interpreter/blockchain.rs

fuel-vm/src/interpreter/executors/instruction.rs

fuel-vm/src/interpreter/contract.rs

fuel-vm/src/tests/blockchain.rs

Dentosal · 2023-10-07T00:25:56Z

fuel-vm/src/tests/blockchain.rs

+            op::move_(reg_a, RegId::HP), // r[a] := $hp
+            op::ldc(0x10, RegId::ZERO, reg_a), /* Load first two words from the
+                                          * contract */


The comment is incorrect here. This loads first $hp bytes from the contract. The code should likely read like this:

Suggested change

op::move_(reg_a, RegId::HP), // r[a] := $hp

op::ldc(0x10, RegId::ZERO, reg_a), /* Load first two words from the

* contract */

op::ldc(0x10, RegId::ZERO, RegId::HP),

Thanks. I think a lot of these are wrong, but I don't really understand them enough to fix them. I'm just copying what was already there.

Might be worth fixing all of them, maybe in another PR.

Dentosal · 2023-10-07T00:27:30Z

fuel-vm/src/tests/blockchain.rs

+            op::ldc(0x10, RegId::ZERO, RegId::ZERO), /* Load first two words from *
+                            * the contract */


This loads zero words from the contract, so the comment is incorrect. I suggest loading a single byte anyway, since this test shouldn't be hitting the zero-length-load corner case.

Suggested change

op::ldc(0x10, RegId::ZERO, RegId::ZERO), /* Load first two words from *

* the contract */

op::ldc(0x10, RegId::ZERO, RegId::ONE),

Dentosal · 2023-10-07T00:30:08Z

fuel-vm/src/tests/blockchain.rs

+        op::move_(reg_a, RegId::SSP),       // r[a] := $ssp
+        op::subi(reg_a, reg_a, 16),         // r[a] -= 16 (start of the loaded code)
        op::xor(reg_b, reg_b, reg_b),       // r[b] := 0
        op::addi(reg_b, reg_b, 16),         // r[b] += 16 (length of the loaded code)


Suggested change

op::move_(reg_a, RegId::SSP), // r[a] := $ssp

op::subi(reg_a, reg_a, 16), // r[a] -= 16 (start of the loaded code)

op::xor(reg_b, reg_b, reg_b), // r[b] := 0

op::addi(reg_b, reg_b, 16), // r[b] += 16 (length of the loaded code)

op::subi(reg_a, RegId::SSP, 16), // r[a] := $ssp - 16 (start of the loaded code)

op::movi(reg_b, 16), // r[b] = 16 (length of the loaded code)

fuel-vm/src/tests/blockchain.rs

xgreenx · 2023-10-09T12:23:31Z

fuel-vm/src/interpreter/blockchain.rs

+                .into_owned()
+                .into();
+        let contract_len = contract_bytes.len();
+        let contract_sub_bytes =


I thought it was already handled by copy_from_slice_zero_fill_noownerchecks.

Yes. And reverting to using this fixes the test bug.

xgreenx · 2023-10-09T12:24:19Z

fuel-vm/src/interpreter/blockchain.rs

+            super::contract::contract(self.storage, &contract_id)?
+                .into_owned()
+                .into();
+        let contract_len = contract_bytes.len();


It would be nice to charge the user here before we do the main work.

I'll look into it. This would be quite the architectural change, I think, but if it's how it needs to work, then I'll make it work.

K. I split the function in half to return the bytes and then copy them separately. I don't love the solution becuase the functions are doing too much, but it's probably better :)

fuel-vm/src/tests/blockchain.rs

…functiond.

xgreenx

@MitchTurner I moved charging for the contract size into the load_contract_code. It works in the same way as for code_size.

If you are okay with it, then you can merge the PR=)

bvrooman · 2023-10-10T14:47:10Z

fuel-vm/src/tests/blockchain.rs

+        op::ldc(reg_a, reg_b, reg_c),  // Load first two words from the contract
+        op::subi(reg_a, RegId::SSP, 16), /* r[a] := $ssp - 16 (start of the loaded
+                                        * code) */
+        op::movi(reg_c, 16), // r[b] = 16 (length of the loaded code)
+        op::logd(RegId::ZERO, RegId::ZERO, reg_a, reg_c), /* Log digest of the


It looks like you're loading len bytes from the contract, given that len is stored in reg_c. In the tests, you pass 0 for len, but the following opcodes hardcode 16. If I understand correctly, ldc still loads the whole contract regardless of length/$rC, but I think the subi, movi, and logd opcodes here become incorrect. Are they even necessary for these tests? Maybe we can remove them.

It fails the ldc__load_external_contract_code test without it.

But you're completely right. I think it just needs to be parameterized for the len. It just so happens that the padded length for that test is 16 🤡

xgreenx · 2023-10-11T09:11:56Z

fuel-vm/src/tests/blockchain.rs

+            op::subi(reg_a, RegId::SSP, padded_len), /* r[a] := $ssp - 16 (start of
+                                                      * the loaded
+                                                      * code) */
+            op::movi(reg_c, padded_len as u32), // r[b] = 16 (length of the loaded code)


Maybe you want to update a comment regarding 16

…dc-cost

MitchTurner added 6 commits October 3, 2023 12:12

Include size lookup of contract for computing cost

80c0e23

Merge remote-tracking branch 'origin/master' into fix-ldc-cost

ce4d94d

Remove comment, update CHANGELOG

8023807

Fix merge mistakes

2e24f6a

Fix ssp_not_sp test

1f6e728

Fix test, cleanup, remove unused code

cec3c68

MitchTurner marked this pull request as ready for review October 5, 2023 01:14

Appease Clippy-sama

5796561

bvrooman reviewed Oct 5, 2023

View reviewed changes

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

fuel-vm/src/interpreter/executors/instruction.rs Outdated Show resolved Hide resolved

fuel-vm/src/tests/blockchain.rs Show resolved Hide resolved

MitchTurner added 5 commits October 5, 2023 14:35

Simplify and reorder code to fix test

716e82c

Remove comment and unnecissary conversion

6a07661

Only take the bytes of the contract that ldc specifies with $rC

429dbe1

Merge branch 'master' into fix-ldc-cost

eaa027b

Add test for copy cost

8053292

MitchTurner added 3 commits October 5, 2023 18:35

Appease Clippy-sama, fix nostd code

da033b0

Call out adherence to spec with comment

d9de157

Fix name, move comment

68afeae

bvrooman reviewed Oct 6, 2023

View reviewed changes

MitchTurner added 3 commits October 6, 2023 10:02

Cleanup

29c5cd8

Refactor tests

fc1ee18

Cleanup

d556e7a

MitchTurner requested a review from a team October 6, 2023 17:54

Dentosal reviewed Oct 7, 2023

View reviewed changes

Dentosal added breaking A breaking api change fuel-vm Related to the `fuel-vm` crate. labels Oct 7, 2023

MitchTurner added 2 commits October 6, 2023 19:54

Fix some of the opcodes in tests

a0008a6

fmt

b71151d

MitchTurner added 3 commits October 8, 2023 14:34

Fix sub_bytes code

3016217

Remove use of std vec

2842fe8

Add offset test

fc837b0

MitchTurner commented Oct 8, 2023

View reviewed changes

fuel-vm/src/tests/blockchain.rs Show resolved Hide resolved

xgreenx reviewed Oct 9, 2023

View reviewed changes

MitchTurner added 2 commits October 9, 2023 11:31

Fix names and reg values, fix bug in interpreter code

6bba267

Add cost right after gettting contract bytes

5ade899

MitchTurner requested a review from a team October 9, 2023 19:21

MitchTurner and others added 3 commits October 9, 2023 12:22

Merge remote-tracking branch 'origin/master' into fix-ldc-cost

51cf8fe

Appease Clippy-sama

f605bb2

Move charging for the size of the contract at the middle of the load …

0b360d6

…functiond.

xgreenx previously approved these changes Oct 10, 2023

View reviewed changes

bvrooman reviewed Oct 10, 2023

View reviewed changes

Fix padding in tests

be21ac2

MitchTurner dismissed xgreenx’s stale review via be21ac2 October 10, 2023 22:28

MitchTurner added 3 commits October 10, 2023 15:30

Fix param names

109af4b

Simplify contract code

ab3d0f2

Merge branch 'master' into fix-ldc-cost

36c0639

xgreenx previously approved these changes Oct 11, 2023

View reviewed changes

MitchTurner added 2 commits October 11, 2023 08:53

Fix comments

000237a

Merge branch 'fix-ldc-cost' of github.com:FuelLabs/fuel-vm into fix-l…

dd971a3

…dc-cost

MitchTurner dismissed xgreenx’s stale review via dd971a3 October 11, 2023 15:53

xgreenx approved these changes Oct 11, 2023

View reviewed changes

MitchTurner added this pull request to the merge queue Oct 11, 2023

Merged via the queue into master with commit 36bf42e Oct 11, 2023
36 checks passed

MitchTurner deleted the fix-ldc-cost branch October 11, 2023 16:09

This was referenced Oct 13, 2023

Release 0.39.0 #608

Merged

Charge properly for the LDC opcode FuelLabs/fuel-core#1271

Closed

xgreenx mentioned this pull request Nov 17, 2023

Charge for the actual size of the contract in ccp opcode #637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include size lookup of contract for computing LDC opcode cost #598

Include size lookup of contract for computing LDC opcode cost #598

MitchTurner commented Oct 3, 2023

MitchTurner commented Oct 6, 2023

Dentosal Oct 7, 2023

MitchTurner Oct 7, 2023

Dentosal Oct 7, 2023

Dentosal Oct 7, 2023

xgreenx Oct 9, 2023

MitchTurner Oct 9, 2023

xgreenx Oct 9, 2023

MitchTurner Oct 9, 2023

MitchTurner Oct 9, 2023

xgreenx left a comment

bvrooman Oct 10, 2023 •

edited

Loading

MitchTurner Oct 10, 2023

xgreenx Oct 11, 2023

		op::ldc(0x10, RegId::ZERO, RegId::ZERO), /* Load first two words from *
		* the contract */

	op::ldc(0x10, RegId::ZERO, RegId::ZERO), /* Load first two words from *
	* the contract */
	op::ldc(0x10, RegId::ZERO, RegId::ONE),

Include size lookup of contract for computing LDC opcode cost #598

Include size lookup of contract for computing LDC opcode cost #598

Conversation

MitchTurner commented Oct 3, 2023

MitchTurner commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

bvrooman Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvrooman Oct 10, 2023 •

edited

Loading