Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blob tx and instructions #780

Merged
merged 45 commits into from
Jul 26, 2024
Merged

Blob tx and instructions #780

merged 45 commits into from
Jul 26, 2024

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Jun 21, 2024

Work towards FuelLabs/fuel-specs#589. Spec PR: FuelLabs/fuel-specs#592. Core PR FuelLabs/fuel-core#1988.

Open for the first round of reviews, but:

  • Typescipt support and tests are still missing
  • Some open questions need to be resolved
    • I commented the PR extensively to mark these

Open questions include:

  • Should BSIZ charge dependent on size? (same for CSIZ as well)
  • Should we implement this for predicates?

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

@Dentosal Dentosal added breaking A breaking api change fuel-vm Related to the `fuel-vm` crate. fuel-asm Related to the `fuel-asm` crate. fuel-tx Related to the `fuel-tx` crate. labels Jun 21, 2024
@Dentosal Dentosal self-assigned this Jun 21, 2024
@Dentosal Dentosal marked this pull request as ready for review June 26, 2024 07:00
@Dentosal Dentosal requested a review from a team June 26, 2024 07:01
bldd: DependentCost::LightOperation {
base: 15,
units_per_gas: 272,
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a fuel-core benches run on reference hardware

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to create an issue in the fuel-core

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fuel-vm/src/tests/coins.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
bldd: DependentCost::LightOperation {
base: 15,
units_per_gas: 272,
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to create an issue in the fuel-core

@@ -1,9 +1,9 @@
use super::*;
/// File generated by fuel-core: benches/src/bin/collect.rs:440. With the following git
/// hash
pub const GIT: &str = "98341e564b75d1157e61d7d5f38612f6224a5b30";
pub const GIT: &str = "98341e564b75d1157e61d7d5f38612f6224a5b30"; // TODO: modified manually
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do it for now. As we discussed during the call, we need to remove default gas costs from the fuel-vm. But it will be a separate PR for separate issue=) We could create a tech-debt for now in the fuel-vm to remove default costs in the future

fuel-tx/src/transaction/types/blob.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/types/blob.rs Show resolved Hide resolved
Revert,
Panic(PanicReason),
GenericFailure(u64),
pub struct AluOk {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to touch it in this PR?=)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RunResult changes must be done here, and these are a natural consequence of those. It would be weird not to do them here.

fuel-vm/src/tests/encoding.rs Outdated Show resolved Hide resolved
fuel-tx/src/tests/offset.rs Outdated Show resolved Hide resolved
fuel-tx/src/tests/offset.rs Show resolved Hide resolved
@Dentosal Dentosal requested a review from xgreenx July 25, 2024 12:54
@Dentosal Dentosal requested a review from Voxelot July 26, 2024 06:48
fuel-tx/src/tests/offset.rs Outdated Show resolved Hide resolved
fuel-tx/src/transaction/types/blob.rs Outdated Show resolved Hide resolved
Comment on lines +769 to +776
let old = storage
.storage_as_mut::<BlobData>()
.replace(blob_id, blob_data.as_ref())
.map_err(RuntimeError::Storage)?;

if old.is_some() {
return Err(InterpreterError::Panic(PanicReason::BlobIdAlreadyUploaded));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will be cheaper to use contains + insert instead of replace. But it can be done in a separate PR/Issue, because other places have the same problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? Also should be profiled if we're doing anything for performance reasons.

fuel-vm/src/tests/blockchain.rs Show resolved Hide resolved
fuel-vm/src/tests/blockchain.rs Outdated Show resolved Hide resolved
fuel-vm/src/tests/blockchain.rs Outdated Show resolved Hide resolved
@Dentosal Dentosal enabled auto-merge July 26, 2024 18:24
@Dentosal Dentosal added this pull request to the merge queue Jul 26, 2024
Merged via the queue into master with commit d2c76e2 Jul 26, 2024
39 checks passed
@Dentosal Dentosal deleted the dento/blob-tx branch July 26, 2024 18:35
Dentosal added a commit to FuelLabs/fuel-specs that referenced this pull request Jul 31, 2024
Work towards #589. The VM
PR FuelLabs/fuel-vm#780 contains a working
implementation.

To allow contracts larger than the contract size limit, and to allow
upgradability, we'll be adding a new tx type that's just a bunch of
bytes, and opcodes to load subsections of that. A *blob* will be like a
contract, but without state or balances, allowing it to be quite a bit
cheaper. The new instructions `BSIZ` and `BLDD` and the new blob mode
for `LDC` closely follow how `CSIZ`, `LDC` and `CCP` work, but only
operation on blob data.

### Before requesting review
- [x] I have reviewed the code myself
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change fuel-asm Related to the `fuel-asm` crate. fuel-tx Related to the `fuel-tx` crate. fuel-vm Related to the `fuel-vm` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants