-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
SMT storage key hashing #1207
SMT storage key hashing #1207
Conversation
We use SMT in two places for contract balances and contract state. While it is not a huge problem for balances SMT root(because `AssetId` is randomly derived from `sha256`), it is a massive problem for contract state root. Each leaf key is specified by the user/developer for the storage key-value pair. The SMT is a vast data structure that uses some optimization that helps to improve its performance and occupied storage. Based on the knowledge of how our SMT works inside, malicious users can manipulate the structure and make it work in a non-optimal way. We've already faced that in the beta3 testnet. [It is a snapshot](https://github.com/FuelLabs/fuel-core/blob/e4f5d65d471954b9cc1148ed067e9bb3f598bb7a/bin/e2e-test-client/src/tests/test_data/large_state/contract.json) of the state of the contract from the beta3 testnet. It has only 30k leafs but because those leafs are close to each other it produces 1.3m of nodes in the SMT. But if we [hash each leaf key](FuelLabs/fuel-core#1207) it reduces the number of the nodes from 1.3m to only 70k. Because of the randomness leafs are distributed in a better way preventing a huge number of empty side nodes. This PR proposes to hash each leaf key of any SMT to prevent any kind of manipulation.
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nits - looks good
.storage::<ContractsAssetsMerkleMetadata>() | ||
.contains_key(contract_id)? | ||
{ | ||
Err(anyhow::anyhow!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd prefer explicit return
over ?
but up to you
.storage::<ContractsStateMerkleMetadata>() | ||
.contains_key(contract_id)? | ||
{ | ||
Err(anyhow::anyhow!("The contract state is already initialized"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
crates/fuel-core/src/service.rs
Outdated
pub fn new(database: Database, mut config: Config) -> anyhow::Result<Task> { | ||
// initialize state | ||
genesis::maybe_initialize_state(&config, &database)?; | ||
config.chain_conf.initial_state = None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the initial_state
before this line? Isn't there any other way for this to get initialized to desired state or it has to go from Some to None in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rollbacked this file because it should be handled separately in the #1209
## What's Changed * version compatibility cleanup by @Voxelot in #1171 * Added example with custom query around the `fuel-core-client` by @xgreenx in #1175 * Update to fuel-vm 0.32 (including wideint gas profiling) by @Dentosal in #1173 * feat: Client primitives by @bvrooman in #1144 * Improve executor config by @Voxelot in #1185 * Added `contract_id` to the `ContractConfig` by @xgreenx in #1184 * fix windows file name error by @firedpeanut in #1176 * Make transaction status stream work by @freesig in #1108 * Added `submit_and_await` endpoint to not miss the notifications by @xgreenx in #1192 * feat: Use ID types in client api by @bvrooman in #1191 * Use `fuel-vm 0.33` with predicate estimation by @xgreenx in #1195 * Add transaction lifecycle diagram to the docs by @digorithm in #1201 * sync with peers before producing blocks by @leviathanbeak in #1169 * SMT storage key hashing by @xgreenx in #1207 ## New Contributors * @firedpeanut made their first contribution in #1176 **Full Changelog**: v0.18.1...v0.19.0 --------- Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
## What's Changed * version compatibility cleanup by @Voxelot in FuelLabs/fuel-core#1171 * Added example with custom query around the `fuel-core-client` by @xgreenx in FuelLabs/fuel-core#1175 * Update to fuel-vm 0.32 (including wideint gas profiling) by @Dentosal in FuelLabs/fuel-core#1173 * feat: Client primitives by @bvrooman in FuelLabs/fuel-core#1144 * Improve executor config by @Voxelot in FuelLabs/fuel-core#1185 * Added `contract_id` to the `ContractConfig` by @xgreenx in FuelLabs/fuel-core#1184 * fix windows file name error by @firedpeanut in FuelLabs/fuel-core#1176 * Make transaction status stream work by @freesig in FuelLabs/fuel-core#1108 * Added `submit_and_await` endpoint to not miss the notifications by @xgreenx in FuelLabs/fuel-core#1192 * feat: Use ID types in client api by @bvrooman in FuelLabs/fuel-core#1191 * Use `fuel-vm 0.33` with predicate estimation by @xgreenx in FuelLabs/fuel-core#1195 * Add transaction lifecycle diagram to the docs by @digorithm in FuelLabs/fuel-core#1201 * sync with peers before producing blocks by @leviathanbeak in FuelLabs/fuel-core#1169 * SMT storage key hashing by @xgreenx in FuelLabs/fuel-core#1207 ## New Contributors * @firedpeanut made their first contribution in FuelLabs/fuel-core#1176 **Full Changelog**: FuelLabs/fuel-core@v0.18.1...v0.19.0 --------- Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
This PR uses an upcoming release of the
fuel-vm 0.34
.It brings a new important feature - hashing the leaf key for the SMT to prevent tree structure manipulation.
As an example: Hashed storage key decreases the number of nodes in the SMT from 1.3M to 70K for 30K leaves in the
run_contract_large_state
e2e test. It improves the test time from 200 seconds to 13 seconds in Debug mode and to 1 second in the release(instead of 20 seconds).So it improves security and performance.
The change is breaking because it affects the
state_root
field -> generatedContractId
.Fixes #1143