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

Disable SMT for ContractsAssets and ContractsState #2048

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Jul 24, 2024

Fixes #2042

Added "smt" feature to the fuel-core-storage to enable old behavior. The SMT still is used in the tests and in benchmarks but disabled for the "production" mode.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

@xgreenx xgreenx requested a review from a team July 24, 2024 21:27
@xgreenx xgreenx self-assigned this Jul 24, 2024
Cargo.toml Outdated Show resolved Hide resolved
@xgreenx xgreenx requested a review from a team July 24, 2024 21:31
&self,
) -> Result<Bytes32, <Database as StorageInspect<ContractsState>>::Error> {
self.database.root(&self.contract_id).map(Into::into)
impl<Database> ContractRef<Database>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is a separate impl block still. The signatures look the same now and were different before. Might be missing something with my visual diff though :)

+ MerkleRootStorage<ContractId, ContractsState, Error = StorageError>
+ MerkleRootStorage<ContractId, ContractsAssets, Error = StorageError>,
{
type InnerError = StorageError;
Copy link
Member

Choose a reason for hiding this comment

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

StorageError seems arbitrary here. Can we just say that it's the same E as the other assoc Errors?

where
D: StorageInspect<ContractsLatestUtxo, Error = StorageError>,
{
type InnerError = StorageError;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

}

impl<'a, Database> ContractRef<&'a Database>
where
Database: ContractStorageTrait,
anyhow::Error: From<Database::InnerError>,
&'a Database: ContractStorageTrait<InnerError = StorageError>,
Copy link
Member

Choose a reason for hiding this comment

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

StorageError seems like an implicit way of getting the right value based on the impls we have above.

E: Into<StorageError>,
&'a Database: ContractStorageTrait<InnerError = E>,

might be more accurate, right?

Copy link
Member

Choose a reason for hiding this comment

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

I think this one is fine. In reality, it looks like we're just using the ContractStorageTrait as a sort of alias for all the other traits. I don't think we're using it anywhere else. But I'm not going to nitpick too much. I guess I'm just not a huge fan of the sort of inheritance-ness going on here.

Cargo.toml Outdated Show resolved Hide resolved
@MitchTurner MitchTurner enabled auto-merge (squash) August 2, 2024 18:35
@MitchTurner MitchTurner merged commit 982270f into master Aug 2, 2024
28 checks passed
@MitchTurner MitchTurner deleted the feature/disable-sparse-merkle-tree branch August 2, 2024 18:35
@MitchTurner MitchTurner mentioned this pull request Aug 2, 2024
@Torres-ssf Torres-ssf restored the feature/disable-sparse-merkle-tree branch August 3, 2024 15:58
@Torres-ssf Torres-ssf deleted the feature/disable-sparse-merkle-tree branch August 5, 2024 11:06
@Torres-ssf Torres-ssf restored the feature/disable-sparse-merkle-tree branch August 5, 2024 15:34
MitchTurner added a commit that referenced this pull request Aug 6, 2024
## What's Changed
* L2 Block Source & Metadata Storage implementations by @MitchTurner in
#1983
* Weekly `cargo update` by @github-actions in
#2029
* Add V0 algorithm to actual services by @MitchTurner in
#2025
* Weekly `cargo update` by @github-actions in
#2039
* Add syncronization for Gas Price Database and On Chain Database on
startup by @MitchTurner in
#2041
* Ignore the message receipt if transaction is reverted by @xgreenx in
#2045
* feat: add chain config to Docker images by @mchristopher in
#2052
* Blob tx support and fuel-vm 0.56.0 by @Dentosal in
#1988
* Disable SMT for `ContractsAssets` and `ContractsState` by @xgreenx in
#2048

## New Contributors
* @mchristopher made their first contribution in
#2052

**Full Changelog**:
v0.31.0...v0.32.0
GoldenPath1109 added a commit to GoldenPath1109/fuel-core that referenced this pull request Sep 7, 2024
## What's Changed
* L2 Block Source & Metadata Storage implementations by @MitchTurner in
FuelLabs/fuel-core#1983
* Weekly `cargo update` by @github-actions in
FuelLabs/fuel-core#2029
* Add V0 algorithm to actual services by @MitchTurner in
FuelLabs/fuel-core#2025
* Weekly `cargo update` by @github-actions in
FuelLabs/fuel-core#2039
* Add syncronization for Gas Price Database and On Chain Database on
startup by @MitchTurner in
FuelLabs/fuel-core#2041
* Ignore the message receipt if transaction is reverted by @xgreenx in
FuelLabs/fuel-core#2045
* feat: add chain config to Docker images by @mchristopher in
FuelLabs/fuel-core#2052
* Blob tx support and fuel-vm 0.56.0 by @Dentosal in
FuelLabs/fuel-core#1988
* Disable SMT for `ContractsAssets` and `ContractsState` by @xgreenx in
FuelLabs/fuel-core#2048

## New Contributors
* @mchristopher made their first contribution in
FuelLabs/fuel-core#2052

**Full Changelog**:
FuelLabs/fuel-core@v0.31.0...v0.32.0
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Sparse Merkle tree usage from the ContractsState and ContractsAssets tables
3 participants