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

alvrs · 2023-09-22T22:39:13Z

Fixes #1449
Fixes #1137
Fixes #1523

Details on the change in the changeset.

More context

setRecord static data, dynamic data lengths, dynamic datas
- The event includes the entire data that is being written to storage (static, packed lengths, dynamic data)
- For static data the onchain way of handling it is the same as offchain
- The provided field layout is used for separating the provided data blob.
  - If the numDynamicFields value in fieldLayout is invalid (e.g. 0), the indexer would override the dynamic field data, while it would stay its previous value onchain.
    - we could solve this by either loading fieldLayout from storage instead of accepting it as a passed in parameter
    - or by emitting fieldLayout as part of the event, so indexers can imitate the onchain behavior
- We “validate the input” onchain, but can actually remove that check, because all we’re testing is
  - provided static data matches the field layout static data length (doesn’t matter, if data is too long or too short it’s handled the same onchain and offchain)
  - check that the encoded total length matches the dynamic data length
    - i’m not sure if we currently do this, but it would be possible to append 0 to the dynamic data blob to mirror the onchain behavior. In this case we don’t need the onchain check
spliceStaticData static data
- onchain deleteCount is always data.length, so we should imitate that behavior offchain
- no need to include deleteCount in the event or function signature
deleteRecord static data, dynamic data lengths
- the user provided field layout determines how much of the static data is actually deleted
  - to solve this, either emit the field layout as part of the event and handle the same way as onchain, or load the field layout from storage
- the user provided field layout determines whether or not we set the dynamic lengths to 0 (= deleting the dynamic data)
  - also, since we don’t actually set the dynamic data to 0, we get invalid data onchain in getFieldSlice after deleting the data
    - this can be solved by adding a bound check for reading slices of fields
    - alternatively we could make it more explicit that users have to check the length before reading from an index, but that would be more expensive (if they decide to do it)
spliceDynamicData dynamic data length, dynamic data ✅
- start depends on user provided field, previous encoded lengths, and startWithin field
  - we load previousEncodedLengths from storage, so can’t be tampered with
  - startWithinField is what we use onchain, and we use the provided field index to find the right storage slot
- we make sure onchain that deleteCount is at the end of the field to guarantee the indexer doesn’t cut into another field or shifts data at the end of the field
- the updated encoded length is emitted with the event and stored onchain

…ison" This reverts commit 31fe746.

changeset-bot · 2023-09-22T22:39:16Z

🦋 Changeset detected

Latest commit: 7893115

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages

Name	Type
@latticexyz/block-logs-stream	Major
@latticexyz/cli	Major
@latticexyz/common	Major
@latticexyz/dev-tools	Major
@latticexyz/store-sync	Major
@latticexyz/store	Major
@latticexyz/world	Major
create-mud	Major
@latticexyz/store-indexer	Major
@latticexyz/config	Major
@latticexyz/protocol-parser	Major
@latticexyz/react	Major
@latticexyz/abi-ts	Major
@latticexyz/ecs-browser	Major
@latticexyz/faucet	Major
@latticexyz/gas-report	Major
@latticexyz/network	Major
@latticexyz/noise	Major
@latticexyz/phaserx	Major
@latticexyz/recs	Major
@latticexyz/schema-type	Major
@latticexyz/services	Major
@latticexyz/solecs	Major
solhint-config-mud	Major
solhint-plugin-mud	Major
@latticexyz/std-client	Major
@latticexyz/std-contracts	Major
@latticexyz/store-cache	Major
@latticexyz/utils	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

alvrs · 2023-09-22T22:47:12Z

packages/store/gas-report.json

-    "gasUsed": 103978
+    "gasUsed": 104908


gas only increased around 1k for set record here (i expected 2k for the additional sload, but seems like some of it was offset by less memory being passed around between functions and the optimized getFieldLayout)

holic · 2023-09-23T20:31:01Z

.changeset/wicked-squids-do.md

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


should we make updateInDynamicField more like a spliceDynamicField function?

there is a spliceDynamicField function too! updateInDynamicField was there before but now basically just calls spliceDynamicField with the correct deleteCount value and does a range check. In fact the range check is not even necessary, since _spliceDynamicField does a range check already. Could argue updateInDynamicField not really necessary.

yeah I am leaning towards us removing, the name was always confusing to me

holic · 2023-09-23T20:37:35Z

.changeset/wicked-squids-do.md

+- `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.


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?

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.

See 004fe9c and the following 3 commits

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

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

holic · 2023-09-23T20:37:47Z

docs/pages/store/spec.mdx

@@ -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),


sorry lermchair

holic · 2023-09-23T20:48:02Z

packages/common/src/getByteLength.ts

@@ -0,0 +1,6 @@
+import { Hex } from "viem";
+
+export function getByteLength(data: Hex): number {


viem has size for this purpose: https://github.com/wagmi-dev/viem/blob/main/src/utils/data/size.ts#L12

oh nice, i figured there must exist something for a super general util like this one

holic · 2023-09-23T20:57:23Z

packages/world/gas-report.json

  },
  {
    "file": "test/WorldDynamicUpdate.t.sol",
-    "test": "testPopFromField",
+    "test": "testPopFromDyanmicField",


Suggested change

"test": "testPopFromDyanmicField",

"test": "testPopFromDynamicField",

holic · 2023-09-23T20:57:33Z

packages/world/gas-report.json

  },
  {
    "file": "test/WorldDynamicUpdate.t.sol",
-    "test": "testPopFromField",
+    "test": "testPopFromDyanmicField",


Suggested change

"test": "testPopFromDyanmicField",

"test": "testPopFromDynamicField",

alvrs added 7 commits September 22, 2023 22:43

add overload for static data location with a single key

e5666fd

optimize get field layout

2b913f4

load field layout from storage

def3f89

try emitting event instead of loading from storage for comparison

31fe746

Revert "try emitting event instead of loading from storage for compar…

a86497d

…ison" This reverts commit 31fe746.

remove field layout from deleteRecord signature

b09468d

remove field layout from setRecord

34f8d29

vercel bot deployed to Preview September 22, 2023 22:39 View deployment

alvrs commented Sep 22, 2023

View reviewed changes

alvrs added 2 commits September 23, 2023 13:56

fix world methods

51692ad

fix modules and gas report

be383d6

vercel bot deployed to Preview September 23, 2023 13:14 View deployment

add internal StoreCore methods that allow passing fieldLayout in

2282499

vercel bot deployed to Preview September 23, 2023 13:28 View deployment

use internal StoreCore methods in tablegen's internal methods

4f7831c

vercel bot deployed to Preview September 23, 2023 14:16 View deployment

alvrs changed the title ~~feat(store): WIP prevent onchain state and indexer state from diverging~~ feat(store): prevent onchain state and indexer state from diverging Sep 23, 2023

remove unnecessary validation

1cf018f

vercel bot deployed to Preview September 23, 2023 15:04 View deployment

alvrs added 2 commits September 23, 2023 16:22

remove deleteCount from SpliceStaticData

6046fb0

integrate new SpliceStaticData event

9729f67

vercel bot deployed to Preview September 23, 2023 15:28 View deployment

alvrs added 6 commits September 23, 2023 16:39

reuse numStaticFields

e452215

actually delete data in deleteRecord

004fe9c

add more meaningful gas report for deleting on cold slot

e818707

add longer dynamic field for test purposes

b703216

verify out of bounds in getFieldSlice

10fbec7

revert if data out of bounds is accessed in dynamic field

74987e1

vercel bot deployed to Preview September 23, 2023 17:20 View deployment

fix e2e

c12617c

vercel bot deployed to Preview September 23, 2023 19:42 View deployment

regenerate test data

9b22c74

vercel bot deployed to Preview September 23, 2023 19:43 View deployment

fix type error in dev tools

fd361df

vercel bot deployed to Preview September 23, 2023 19:47 View deployment

Create wicked-squids-do.md

e28b0a0

vercel bot deployed to Preview September 23, 2023 20:26 View deployment

holic reviewed Sep 23, 2023

View reviewed changes

expect revert when accessing out of bound

c79eec2

vercel bot deployed to Preview September 23, 2023 20:35 View deployment

holic reviewed Sep 23, 2023

View reviewed changes

remove updateInField

5a29de7

vercel bot deployed to Preview September 23, 2023 21:26 View deployment

alvrs added 2 commits September 23, 2023 22:31

Update wicked-squids-do.md

d9fdbf9

update gas report

05eb31b

vercel bot deployed to Preview September 23, 2023 21:34 View deployment

prettier

1d3adb8

vercel bot deployed to Preview September 23, 2023 21:35 View deployment

replace custom function with size

7893115

alvrs marked this pull request as ready for review September 23, 2023 21:38

alvrs requested a review from dk1a as a code owner September 23, 2023 21:38

vercel bot deployed to Preview September 23, 2023 21:38 View deployment

holic approved these changes Sep 23, 2023

View reviewed changes

alvrs merged commit cea754d into main Sep 23, 2023

alvrs deleted the alvrs/store-polish branch September 23, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alvrs commented Sep 22, 2023 •

edited

Loading

changeset-bot bot commented Sep 22, 2023 •

edited

Loading

alvrs Sep 22, 2023

holic Sep 23, 2023 •

edited

Loading

alvrs Sep 23, 2023

holic Sep 23, 2023

holic Sep 23, 2023 •

edited

Loading

alvrs Sep 23, 2023 •

edited

Loading

alvrs Sep 23, 2023

holic Sep 23, 2023

alvrs Sep 23, 2023

holic Sep 23, 2023

holic Sep 23, 2023

alvrs Sep 23, 2023

holic Sep 23, 2023

holic Sep 23, 2023

		@@ -0,0 +1,6 @@
		import { Hex } from "viem";

		export function getByteLength(data: Hex): number {

	"test": "testPopFromDyanmicField",
	"test": "testPopFromDynamicField",

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

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

Conversation

alvrs commented Sep 22, 2023 • edited Loading

changeset-bot bot commented Sep 22, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

holic Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

alvrs Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvrs commented Sep 22, 2023 •

edited

Loading

changeset-bot bot commented Sep 22, 2023 •

edited

Loading

holic Sep 23, 2023 •

edited

Loading

holic Sep 23, 2023 •

edited

Loading

alvrs Sep 23, 2023 •

edited

Loading