fuel-core v0.13 upgrade #3181
Merged
fuel-core v0.13 upgrade #3181
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
This is amazing, thank you @Voxelot. Everything here seems very reasonable to me.
I suppose once we figure out how to use $rB, we have to expose it in IR as an output to state_read_* and state_write_* instructions.
Voxelot
commented
Oct 28, 2022
Co-authored-by: Mohammad Fawaz <mohammadfawaz89@gmail.com>
# Conflicts: # Cargo.lock # forc-pkg/Cargo.toml # forc-plugins/forc-client/Cargo.toml # test/src/e2e_vm_tests/mod.rs
Voxelot
added a commit
to FuelLabs/fuels-rs
that referenced
this pull request
Nov 1, 2022
WIP to upgrade to fuel-core 0.13 with the new generic tx format sway tests were compiled using forc from this PR: FuelLabs/sway#3181 Because there were breaking changes to the fuel-vm opcodes, we have to check in the compiled sway files for now until a new version of forc can be released. However, releasing forc is blocked on the SDK supporting fuel-core 0.13, so the upgrades were done concurrently. Co-authored-by: green <xgreenx9999@gmail.com>
mohammadfawaz
previously approved these changes
Nov 2, 2022
mohammadfawaz
previously approved these changes
Nov 2, 2022
mohammadfawaz
approved these changes
Nov 2, 2022
JoshuaBatty
reviewed
Nov 2, 2022
| u_128 = U128::from(0, 1); | ||
| root_of_u_128 = u_128.sqrt(); | ||
| assert(root_of_u_128 == U128::from(0, 1)); | ||
| //let mut u_128: U128 = U128::from(0, 49); |
There was a problem hiding this comment.
is this block of code meant to be commented out for this PR?
There was a problem hiding this comment.
Yeah. The operation is now too expensive with the new gas schedule and the test was running out of gas. The implementation should probably be revamped. It's a new feature anyways (like 2 days ago).
JoshuaBatty
approved these changes
Nov 2, 2022
mohammadfawaz
approved these changes
Nov 2, 2022
This was referenced Nov 2, 2022
MujkicA
pushed a commit
to FuelLabs/fuels-rs
that referenced
this pull request
Nov 4, 2022
WIP to upgrade to fuel-core 0.13 with the new generic tx format sway tests were compiled using forc from this PR: FuelLabs/sway#3181 Because there were breaking changes to the fuel-vm opcodes, we have to check in the compiled sway files for now until a new version of forc can be released. However, releasing forc is blocked on the SDK supporting fuel-core 0.13, so the upgrades were done concurrently. Co-authored-by: green <xgreenx9999@gmail.com>
mohammadfawaz
added a commit
that referenced
this pull request
Nov 5, 2022
Once we have a fully fledged instruction combining optimization pass, these types of optimizations won't be needed anymore (for the most part). For now, this allows us to re-enable the `u128_root_test` that was disabled in #3181. * Switch division by 2 to a shift left by 1 * Switch to using the lower part of a `U128` in situations where the upper part is obviously not impacted. This cuts down the gas cost for `sqrt` by around 30-40% and the `u128_root_test` now consumes ~77M gas. Previously, the test was running out of gas (our default limit is 100M). I also made the same optimization in `U256` but notice that the `u256_div_test` was disabled (doesn't have a `test.toml`!) so I enabled. The test was actually running out of gas before my change but now it just barely passes with like 98M gas, which is dangerous. Finally, I replaced the method of multiplication in both `U128` and `U256` and enabled two previously disabled tests: * `u256_mul_test` which used to run out of gas but now takes ~21M (so that's a least 5 X improvement). * `u256_ops_test` which runs fine. In short, div and mul for both `U128` and `U256` are now much cheaper and so is `sqrt` for `U128` (`U256` doesn't have a `sqrt` yet). Closes #3256 Closes #3245 Co-authored-by: João Matos <joao@tritao.eu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
currently blocked on fuel-rs upgrade
Closes #3159
Closes #2115 (the dependency is on
sway-corenow)Had to disable the u128 sqrt tests for now as they are too expensive / slow, as per @mohammadfawaz suggestion.