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

feat(store): prevent onchain state and indexer state from diverging #1581

Merged
merged 38 commits into from
Sep 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e5666fd
add overload for static data location with a single key
alvrs Sep 22, 2023
2b913f4
optimize get field layout
alvrs Sep 22, 2023
def3f89
load field layout from storage
alvrs Sep 22, 2023
31fe746
try emitting event instead of loading from storage for comparison
alvrs Sep 22, 2023
a86497d
Revert "try emitting event instead of loading from storage for compar…
alvrs Sep 22, 2023
b09468d
remove field layout from deleteRecord signature
alvrs Sep 22, 2023
34f8d29
remove field layout from setRecord
alvrs Sep 22, 2023
51692ad
fix world methods
alvrs Sep 23, 2023
be383d6
fix modules and gas report
alvrs Sep 23, 2023
2282499
add internal StoreCore methods that allow passing fieldLayout in
alvrs Sep 23, 2023
4f7831c
use internal StoreCore methods in tablegen's internal methods
alvrs Sep 23, 2023
1cf018f
remove unnecessary validation
alvrs Sep 23, 2023
6046fb0
remove deleteCount from SpliceStaticData
alvrs Sep 23, 2023
9729f67
integrate new SpliceStaticData event
alvrs Sep 23, 2023
e452215
reuse numStaticFields
alvrs Sep 23, 2023
004fe9c
actually delete data in deleteRecord
alvrs Sep 23, 2023
e818707
add more meaningful gas report for deleting on cold slot
alvrs Sep 23, 2023
b703216
add longer dynamic field for test purposes
alvrs Sep 23, 2023
10fbec7
verify out of bounds in getFieldSlice
alvrs Sep 23, 2023
74987e1
revert if data out of bounds is accessed in dynamic field
alvrs Sep 23, 2023
05ac89d
before refacor of all usages
alvrs Sep 23, 2023
2b84cf6
refactor except tests
alvrs Sep 23, 2023
5ffbbd7
refactor tests and gas-report
alvrs Sep 23, 2023
efcec63
expose methods to explicitly set static or dynamic fields
alvrs Sep 23, 2023
57b8cd5
use setStaticField or setDynamicField in tablegen
alvrs Sep 23, 2023
a503e72
add overloads for getRecord, getField, getFieldLength and setField th…
alvrs Sep 23, 2023
5df9270
use getDynamicFieldLength in codegen
alvrs Sep 23, 2023
8a05518
regenerate artifacts
alvrs Sep 23, 2023
c12617c
fix e2e
alvrs Sep 23, 2023
9b22c74
regenerate test data
alvrs Sep 23, 2023
fd361df
fix type error in dev tools
alvrs Sep 23, 2023
e28b0a0
Create wicked-squids-do.md
alvrs Sep 23, 2023
c79eec2
expect revert when accessing out of bound
alvrs Sep 23, 2023
5a29de7
remove updateInField
alvrs Sep 23, 2023
d9fdbf9
Update wicked-squids-do.md
alvrs Sep 23, 2023
05eb31b
update gas report
alvrs Sep 23, 2023
1d3adb8
prettier
alvrs Sep 23, 2023
7893115
replace custom function with size
alvrs Sep 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions .changeset/wicked-squids-do.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
---
"@latticexyz/block-logs-stream": patch
"@latticexyz/cli": patch
"@latticexyz/common": minor
"@latticexyz/dev-tools": patch
"@latticexyz/store-sync": patch
"@latticexyz/store": major
"@latticexyz/world": major
"create-mud": patch
---

- The external `setRecord` and `deleteRecord` methods of `IStore` no longer accept a `FieldLayout` as input, but load it from storage instead.
This is to prevent invalid `FieldLayout` values being passed, which could cause the onchain state to diverge from the indexer state.
However, the internal `StoreCore` library still exposes a `setRecord` and `deleteRecord` method that allows a `FieldLayout` to be passed.
This is because `StoreCore` can only be used internally, so the `FieldLayout` value can be trusted and we can save the gas for accessing storage.

```diff
interface IStore {
function setRecord(
ResourceId tableId,
bytes32[] calldata keyTuple,
bytes calldata staticData,
PackedCounter encodedLengths,
bytes calldata dynamicData,
- FieldLayout fieldLayout
) external;

function deleteRecord(
ResourceId tableId,
bytes32[] memory keyTuple,
- FieldLayout fieldLayout
) external;
}
```

- The `spliceStaticData` method and `Store_SpliceStaticData` event of `IStore` and `StoreCore` no longer include `deleteCount` in their signature.
This is because when splicing static data, the data after `start` is always overwritten with `data` instead of being shifted, so `deleteCount` is always the length of the data to be written.

```diff

event Store_SpliceStaticData(
ResourceId indexed tableId,
bytes32[] keyTuple,
uint48 start,
- uint40 deleteCount,
bytes data
);

interface IStore {
function spliceStaticData(
ResourceId tableId,
bytes32[] calldata keyTuple,
uint48 start,
- uint40 deleteCount,
bytes calldata data
) external;
}
```

- The `updateInField` method has been removed from `IStore`, as it's almost identical to the more general `spliceDynamicData`.
If you're manually calling `updateInField`, here is how to upgrade to `spliceDynamicData`:

```diff
- store.updateInField(tableId, keyTuple, fieldIndex, startByteIndex, dataToSet, fieldLayout);
+ uint8 dynamicFieldIndex = fieldIndex - fieldLayout.numStaticFields();
+ store.spliceDynamicData(tableId, keyTuple, dynamicFieldIndex, uint40(startByteIndex), uint40(dataToSet.length), dataToSet);
```

- All other methods that are only valid for dynamic fields (`pushToField`, `popFromField`, `getFieldSlice`)
have been renamed to make this more explicit (`pushToDynamicField`, `popFromDynamicField`, `getDynamicFieldSlice`).

Their `fieldIndex` parameter has been replaced by a `dynamicFieldIndex` parameter, which is the index relative to the first dynamic field (i.e. `dynamicFieldIndex` = `fieldIndex` - `numStaticFields`).
The `FieldLayout` parameter has been removed, as it was only used to calculate the `dynamicFieldIndex` in the method.

```diff
interface IStore {
- function pushToField(
+ function pushToDynamicField(
ResourceId tableId,
bytes32[] calldata keyTuple,
- uint8 fieldIndex,
+ uint8 dynamicFieldIndex,
bytes calldata dataToPush,
- FieldLayout fieldLayout
) external;

- function popFromField(
+ function popFromDynamicField(
ResourceId tableId,
bytes32[] calldata keyTuple,
- uint8 fieldIndex,
+ uint8 dynamicFieldIndex,
uint256 byteLengthToPop,
- FieldLayout fieldLayout
) external;

- function getFieldSlice(
+ function getDynamicFieldSlice(
ResourceId tableId,
bytes32[] memory keyTuple,
- uint8 fieldIndex,
+ uint8 dynamicFieldIndex,
- FieldLayout fieldLayout,
uint256 start,
uint256 end
) external view returns (bytes memory data);
}
```

- `IStore` has a new `getDynamicFieldLength` length method, which returns the byte length of the given dynamic field and doesn't require the `FieldLayout`.

```diff
IStore {
+ function getDynamicFieldLength(
+ ResourceId tableId,
+ bytes32[] memory keyTuple,
+ uint8 dynamicFieldIndex
+ ) external view returns (uint256);
}

```

- `IStore` now has additional overloads for `getRecord`, `getField`, `getFieldLength` and `setField` that don't require a `FieldLength` to be passed, but instead load it from storage.

- `IStore` now exposes `setStaticField` and `setDynamicField` to save gas by avoiding the dynamic inference of whether the field is static or dynamic.

- The `getDynamicFieldSlice` method no longer accepts reading outside the bounds of the dynamic field.
This is to avoid returning invalid data, as the data of a dynamic field is not deleted when the record is deleted, but only its length is set to zero.
Copy link
Member

@holic holic Sep 23, 2023

Choose a reason for hiding this comment

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

I'm curious about the gas difference between actually deleting and setting length to zero - isn't there a gas return when freeing storage slots?

Copy link
Member Author

@alvrs alvrs Sep 23, 2023

Choose a reason for hiding this comment

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

I thought so too and actually implemented the actual removal today. Surprisingly setting it to 0 was a lot more expensive, and the cost grew linearly. Eventually I realized that you still have to provide the entire gas for the transaction to complete, and at the end of the tx receive up to 20% refund. So in the end it's almost always cheaper for us to not touch the dynamic data at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

See 004fe9c and the following 3 commits

Copy link
Member

Choose a reason for hiding this comment

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

sorry ethereum for the state bloat we're about to create

Copy link
Member Author

Choose a reason for hiding this comment

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

that's what ethereum gets for being so stingy with the refund

2 changes: 1 addition & 1 deletion docs/pages/store/spec.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ When a record or a single field is edited, or when a record is deleted, Store em

```solidity
event Store_SetRecord(bytes32 indexed tableId, bytes32[] keyTuple, bytes staticData, bytes32 encodedLengths, bytes dynamicData),
event Store_SpliceStaticData(bytes32 indexed tableId, bytes32[] keyTuple, uint48 start, uint40 deleteCount, bytes data),
event Store_SpliceStaticData(bytes32 indexed tableId, bytes32[] keyTuple, uint48 start, bytes data),
Copy link
Member

Choose a reason for hiding this comment

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

sorry lermchair

event Store_SpliceDynamicData(bytes32 indexed tableId, bytes32[] keyTuple, uint48 start, uint40 deleteCount, bytes data, bytes32 encodedLengths),
event Store_DeleteRecord(bytes32 indexed tableId, bytes32[] keyTuple),
```
Expand Down
24 changes: 12 additions & 12 deletions e2e/packages/contracts/src/codegen/tables/Multi.sol

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions e2e/packages/contracts/src/codegen/tables/Number.sol

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading