Stateful Binary Patricia Contracts#140

Closed

ben-chain wants to merge 12 commits intomasterfrom

feat/YAS380/stateless-patricia-trie

Collaborator

ben-chain commented May 20, 2020

Description

Partially addresses YAS-380, adding a stateless State Manager, by providing a full tree. Handles insertion, retrieval, inclusion, and non-inclusion proofs. Next step is to change the storage format to work statelessly by storing sibling hashes instead of full sibling pairs.

Questions

Still need to measure performance of a big inclusion proof, will do that before merging
Maybe this should go in ovm package?
Hashing is likely different than what we'll end up with, but it's abstracted out so probably best to wait for govm progress.

Metadata

Fixes

Fixes # 380

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

ben-chain added 11 commits

May 12, 2020 18:49


          first pass pulled in existing stuff

90d87b3


          remove arbitrary length keys

6a726ea


          remove Non-incl hashing, test with broken non inclusion verification

7cec4c8


          commenting main functions to confirm understanding

e13585a


          break out edge verification from leaf verification

56a06a4


          new non incl verification working hardcoded)

e941032


          NON INCLUSION FINALLY WORKS

6c41fb3


          naming

295a9f3


          improve test coverage

f483fae


          fix async missinig test

b5b48ff


          linting

91f6732

willmeister reviewed

View reviewed changes

willmeister left a comment

Left a few suggestions to make it read more clearly, but I think everything here makes sense. Would help to list out what the code is supposed to do in function headers.

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated

Comment on lines 252 to 253

		bytes32 conflictingEdgeFullLabelData,
		uint conflictingEdgeFullLabelLength,

willmeister May 21, 2020

Any reason that full is used here and in other variables in this function? Can we assume it is full unless otherwise specified?

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol

+                      // first, verify there is a conflict between the key and given edge
+                      require(conflictingEdgeFullLabelLength <= 256, 'invalid label specified');
+                      D.Label memory fullConflictingEdgeLabel = D.Label(conflictingEdgeFullLabelData, conflictingEdgeFullLabelLength);
+                      D.Label memory fullLeafLabel = D.Label(key, 256);

willmeister May 21, 2020

Leaf labels should always be full, right?

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

packages/rollup-contracts/contracts/state-tree/BinaryUtils.sol Outdated Show resolved Hide resolved

willmeister reviewed

View reviewed changes

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Outdated Show resolved Hide resolved

willmeister reviewed

View reviewed changes

packages/rollup-contracts/contracts/state-tree/BinaryUtils.sol Outdated Show resolved Hide resolved

willmeister reviewed

View reviewed changes

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol Show resolved Hide resolved

willmeister reviewed

View reviewed changes

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol

+                      D.Edge memory e,
+                      D.Label memory key, bytes32 value
+                  ) private returns (D.Edge memory) {
+                      require(key.length >= e.label.length, "Key lenght mismatch label lenght");

willmeister May 22, 2020

Suggested change

      
                    require(key.length >= e.label.length, "Key lenght mismatch label lenght");
          
                    require(key.length >= e.label.length, "Trying to insert at edge with key length < label length");

willmeister reviewed

View reviewed changes

packages/rollup-contracts/contracts/state-tree/FullPatriciaTree.sol

+                  function _insertAtEdge(
+                      Tree storage tree,
+                      D.Edge memory e,
+                      D.Label memory key, bytes32 value

willmeister May 22, 2020

Suggested change

      
                    D.Label memory key, bytes32 value
          
                    D.Label memory key, 
          
                    bytes32 value


          incorporate PR feedback

f9d1597

Collaborator Author

ben-chain commented Jun 12, 2020

Closing, as we determined we need a more memory/storage efficient solution and are rewriting!

ben-chain closed this

gakonst deleted the feat/YAS380/stateless-patricia-trie branch

March 18, 2021 15:01

snario pushed a commit that referenced this pull request


          Code freeze audit integrations (#163)

d34a12f

* Separates storage from SCC and CTC (#151)

* First pass version

* More minor tweaks for tests to pass

* Add authentication

* Minor config updates

* Fix lint error

* Fix naming changes per review

* Enable Deployer Whitelist (#119)

* first pass, test runner updated

* add ability to only validate flag, test passes

* all tests passing

* clean up console.logs

* enforce gas refund preservation

* more cleanup/import removal

* whitelisted -> allowed

* first pass, test runner updated

* add ability to only validate flag, test passes

* all tests passing

* clean up console.logs

* enforce gas refund preservation

* more cleanup/import removal

* whitelisted -> allowed

* remove whitespace

* Restrict StateTransitionerFactory (#140)

* added msg sender check

* add create test

* cleanup

* add param

* add addressmanager.address param

* CTC Chain Monotonicity Fixes (#93)

* [wip] Fix block time logic

* some sad path and happy tests passing

* more progress

* first pass sad cases tested

* cleanup, adding empty tests

* more reversion tests

* rename shouldstartat}

* add final couple tests

* enable more tests

* cleanup

* remove .only

* textual cleanup

* make queue length public

* improve structure, comments

* update deploy config

* address nits

Co-authored-by: Karl Floersch <karl@karlfloersch.com>

* fix declarations, lint (#152)

* Adds river's new Merkle tree implementation, with some cleanup (#148)

* Reverts an accidental breaking merge

* Added new merkle tree impl

* add comments

* Final cleanups and merge

Co-authored-by: Ben Jones <ben@pseudonym.party>

* Fix run gas Lower Bound (#94)

* added the check

* add test

* lower OVM TX size for Kovan

* re-remove gas check

* update gas vals slightly

* lint

* lint

* Merge master into freeze integration branch  (#153)

* update solidity version to ^0.7.0 (#122)

* update solc version to ^0.7.0

* interfaces back to solidity >0.6.0 <0.8.0

* update solc to 0.7.6

* back to 0.7.4

* upgrade to 0.7.6, fix EXTCODESIZE check

* versions >0.5.0 <0.8.0 for xdomain msgers

* ctc: disable appendQueueBatch (#150)

* ctc: disable appendSequencerBatch

* typo: fix

* re-enable verifyQueueTransaction test:

* add explicit test for verifying queue elements against either append

Co-authored-by: Ben Jones <ben@pseudonym.party>

* fix up test

* remove .only

Co-authored-by: Alina <alina@optimism.io>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

* add check, simple test, update deploy (#154)

* go back to first name (#155)

* lint

* fix js number error

* add error logging to help debug deploy

* [code freeze] Fix deploy script (#156)

* fix deploy script

* add block time config

* ensure value is integer

* lint

* remove console logs from deploy

* Moves gas check to applyTransaction (#161)

* move to OVM_ST, pass test

* remove old test because functionality moved

* linting

* remove leaf hasing

* use safe EXEMRG wrapper (#162)

* use safeREQUIRE

* add owner getter

* relayer: add to config (#160)

* relayer: add to config

* lint: fix

* Fix minor error in test config

Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com>
Co-authored-by: ben-chain <ben@pseudonym.party>
Co-authored-by: Alina <alina@optimism.io>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>

protolambda added a commit to protolambda/optimism that referenced this pull request


          Merge pull request ethereum-optimism#140 from ethereum-optimism/m/fix…

6cf5e20

…-dummy-contract

Fix Deposit Feed test

OptimismBot pushed a commit that referenced this pull request


          feat: integrate system config with superchain config (#140)

b240094

* feat: integrate portal to lockbox

* fix: pr fixes

* test: refactor assert

* feat: integrate system config with superchain config

* fix: remove OPCM interop

* test: add dependency counter test

blockchaindevsh pushed a commit to blockchaindevsh/optimism that referenced this pull request


          CircleCI: fix go-tests / DencunBlobTx (ethereum-optimism#140)

32380c2

* fix TestDencunBlobTxInclusion

* fix DencunBlobTx

mslipper added a commit that referenced this pull request


          feat: introduce SharedLockbox (#14199)

31a9c29

* feat: add shared lockbox (#126)

* feat: create shared lockbox contract with its interface and unit tests

* chore: polish tests and interfaces

* chore: run pre-pr

* chore: improve natspec

* chore: run pre-pr

* chore: update compiler version

* feat: integrate portal to lockbox (#139)

* feat: integrate portal to lockbox

* fix: pr fixes

* test: refactor assert

* feat: add liquidity migrator contract with its unit test and interface (#128)

* feat: create shared lockbox contract with its interface and unit tests

* chore: polish tests and interfaces

* chore: run pre-pr

* chore: improve natspec

* chore: run pre-pr

* feat: add liqudity migrator contract with its unit test and interface

* chore: remove underscore on stack var

* chore: add todo

* chore: run pre-pr

* chore: add contract title natspec and proxied

* refactor: integrate testing suite with common test

* chore: pre-pr

* chore: add spec test

* feat: integrate system config with superchain config (#140)

* feat: integrate portal to lockbox

* fix: pr fixes

* test: refactor assert

* feat: integrate system config with superchain config

* fix: remove OPCM interop

* test: add dependency counter test

* feat: manage dependency set on superchain config (#138)

* chore: add zero dependencies check (#142)

* fix: pre pr

* feat: Add pause check (#145)

* feat: Add pause check

Co-authored-by: 0xParticle <particle@defi.sucks>
Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com>
Co-authored-by: Joxess <joxes@defi.sucks>

* test: add tests natspecs

---------

Co-authored-by: 0xParticle <particle@defi.sucks>
Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com>
Co-authored-by: Joxess <joxes@defi.sucks>

* fix: pre pr and interfaces imports

* feat: add upgrader role to superchain config (#163)

* feat: use superchain config lockbox in portal (#164)

* feat: use superchain config lockbox in portal

* test: add new sharedlockbox test

* fix: pre pr

* feat: liquidity migrator deployment (#166)

* feat: liquidity migrator deployment

* test: fix comment

* test: fix internal variables names

* feat: dependency set refactor (#170)

* feat: dependency set refactor

* fix: deploy script variable name

* fix: pr

* fix: pr

* fix: pre pr

* fix: semgrep

* fix: merge conflict

* [WIP] feat: new lockbox (#192)

* chore: partial implementation comments

* feat: new lockbox

* feat: introduce dependency manager predeploy

* feat: remove timestamp check from CrossL2Inbox

* feat: introduce cluster manager role and remove immutables

* fix: remove unnecessary code, fix tests and setup

* feat: use unstructured storage and OZ v5

* fix: L2ToL2CDM dependency set check

* fix: dependency manager gas limit

* feat: refactor interop feature contracts (#200)

* feat: refactor interop feature contracts

* fix: add noops comment

* feat: adds OptimismPortal migrated flag

* test: add missing tests

* fix: portal interop storage naming

---------

Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com>

* fix: pre pr, setup and tests

* fix: remove system config interop and add interop portal target check (#205)

* fix: remove system config interop and add interop portal check

* fix: interop portal target check order

* fix: remove wrong comment

* fix: refactor portal noops function (#206)

* test: add dependency manager and portal interop tests (#209)

* feat: initialize shared lockbox in interop portal (#211)

* feat: initialize shared lockbox in interop portal

* fix: refactor shared lockbox storage getter

* fix: lockbox pr fixes (#214)

* fix: pre pr

* fix: semver lock

* fix: semver lock

* feat: descope dependency manager (#242)

* feat: descope dependency manager

* test: fix tests

* test: fix tests

* chore: improve eth liquidity test (#248)

* fix: internal review fixes (#243)

* fix: I-0

* fix: I-1

* fix: I-2

* fix: I-3

* fix: I-6

* fix: I-7

* fix: I-9

* fix: pre pr

* fix: pre pr

* fix: portal withdrawal checks (#255)

* fix: portal withdrawal checks

* fix: include current withdrawal check

* fix: remove unused interop contracts (#256)

* test: fix flake tests (#257)

* fix: adjust op-deployer interop scripts (#262)

* fix: pre pr

* fix op-deployer tests

* remove dependency code

* fix lint

* use Bob account

---------

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>
Co-authored-by: AgusDuha <81362284+agusduha@users.noreply.github.com>
Co-authored-by: agusduha <agusnduha@gmail.com>
Co-authored-by: 0xParticle <particle@defi.sucks>
Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com>
Co-authored-by: Joxess <joxes@defi.sucks>
Co-authored-by: Skeletor Spaceman <skeletorspaceman@gmail.com>

Zena-park added a commit to tokamak-network/optimism that referenced this pull request


          [SMR-02M] Potential of Data Corruption ethereum-optimism#131 ethereum…

aa3534f

…-optimism#140 ethereum-optimism#146

Zena-park added a commit to tokamak-network/optimism that referenced this pull request


          [SMV-02M] Potential of Data Corruption ethereum-optimism#140

2eeb67a

theochap pushed a commit that referenced this pull request


          chore: v0.3.3 (#140)

4162c2f

### Description

`v0.3.3`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet