Use ArbOS chain config as the cannonial chain config.

zacshowa commented 1 month ago

Closes #228

This PR:

Adds a method for nitro nodes, primarily the sequencer, to read the ArbOS chain config when determining if espresso is enabled. This allows the EnableEspresso field of the chain config to act as a live switch to enable espresso independently of any upgrade.

Key places to review:

Key files to review are transaction_streamer.go, sequencer.go, executionengine.go, and node.go are the primary places to review, changes to interface.go are important as well.

How to test this PR:

Testing this PR also involves using the work in the set-chain-config branch of the nitro-testnode repo here

You can either check this branch out in a different repository, or likely point the nitro-testnode submodule in this repo at that branch.

My testing process included the following steps:

Run docker build . -t espresso-integration-testnode in this repo checked out to this branch.
Run nitro-testnode/espresso-tests/migration-test.bash to run the migration test.
If the migration test finishes, everything is in working order.

There should be some output near the end of the test that looks similar to this:

+ '[' 119 '!=' 200 ']'
++ curl http://localhost:41000/v0/availability/block/119/namespace/412346
++ jq -r '.transactions.[0].namespace'
++ tail -n 1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   732  100   732    0     0   373k      0 --:--:-- --:--:-- --:--:--  714k
+ export namespace=412346
+ namespace=412346
+ '[' 412346 -eq 412346 ']'
+ echo 'ChainID matches chainID from transaction stored in Espresso network!, chain config has been successfully set.'
ChainID matches chainID from transaction stored in Espresso network!, chain config has been successfully set.
+ break
+ cd /home/nixnovice/espresso/nitro-testnode/orbit-actions
++ cast call 0x784FC11476F3d06801A76b944795E6367391b12e 'osp()(address)' --rpc-url http://localhost:8545
+ CHALLENGE_MANAGER_OSP_ADDRESS=0x773D62Ce1794b11788907b32F793e647A4f9A1F7
+ '[' 0x773D62Ce1794b11788907b32F793e647A4f9A1F7 '!=' 0x773D62Ce1794b11788907b32F793e647A4f9A1F7 ']'
+ RECIPIENT_ADDRESS=0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
++ cast balance 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA -e --rpc-url http://localhost:8547
+ BALANCE_ORIG=0.000000000000000000
+ cast send 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA --value 1ether --rpc-url http://localhost:8547 --private-key 0xdc04c5399f82306ec4b4d654a342f40e2e0620fe39950d967e1e574b32d4dd36

This shows that the chain config has switched as we are successfully forwarding transactions to the espresso confirmation layer in the test.

Things tested:

I have confirmed that locally this change is working with the testnode and is showing the chain config switching.

zacshowa commented 1 month ago

can you add a test to see if this works? We might also have to make changes to the replay binary to check whether espresso is enabled

My intention was for the migration test to be the test of this work and I intend to have that finished soon.

zacshowa commented 1 month ago

The migration test is now complete in the branch linked in the PR description. LMK if there is context I'm forgetting to give!

zacshowa commented 1 month ago

@Sneh1999

I have a few suggestions for changes:

1. [replay binary](https://github.com/EspressoSystems/nitro-espresso-integration/blob/integration/cmd/replay/main.go#L290-L326) should be modified?

2. `EnableEspressoSovereign` flag needs to be replaced by this check?

3. Is the `EnableEspresso` flag in `ArbitrumChainParams` needed now?

4. I understand that we added the migration test to `nitro-testnode` but should we not add any test for `GetArbOSConfigAtHeight`  and espressoSwitch - reasoning is that someone might break this functionality without knowing.
   Let me know your thoughts :)

The replay binary already loads the arbOS config from the current block to check the chainConfig, so I don't think we need changes there.
EnableEspressoSovereign is currently only used as a "creation time" config check here
We do need something in the ArbitrumChainParams to refer to when deserializing and using the deserialized config. I think the coupling with the actual go struct is loose. I.E. you can set something in the ArbOS chain config that doesn't exsist in the go struct and the transaction won't revert, but I don't think we should remove this param at all. Otherwise I think we will have issues with deserialization. Furthermore we might change it as per the comments left by Mathis.
I think we could add an integration test for the arbOS config fetching with this implementation somewhat easily, the test would remain relatively the same too if we don't change the current API in the execution node. I'm not sure how we could easily test espressoSwitch()

Sneh1999 commented 1 month ago

@Sneh1999
I have a few suggestions for changes:
1. [replay binary](https://github.com/EspressoSystems/nitro-espresso-integration/blob/integration/cmd/replay/main.go#L290-L326) should be modified?

2. `EnableEspressoSovereign` flag needs to be replaced by this check?

3. Is the `EnableEspresso` flag in `ArbitrumChainParams` needed now?

4. I understand that we added the migration test to `nitro-testnode` but should we not add any test for `GetArbOSConfigAtHeight`  and espressoSwitch - reasoning is that someone might break this functionality without knowing.
   Let me know your thoughts :)
The replay binary already loads the arbOS config from the current block to check the chainConfig, so I don't think we need changes there.

EnableEspressoSovereign is currently only used as a "creation time" config check here

We do need something in the ArbitrumChainParams to refer to when deserializing and using the deserialized config. I think the coupling with the actual go struct is loose. I.E. you can set something in the ArbOS chain config that doesn't exsist in the go struct and the transaction won't revert, but I don't think we should remove this param at all. Otherwise I think we will have issues with deserialization. Furthermore we might change it as per the comments left by Mathis.

I think we could add an integration test for the arbOS config fetching with this implementation somewhat easily, the test would remain relatively the same too if we don't change the current API in the execution node. I'm not sure how we could easily test espressoSwitch()

okay maybe the naming is confusing lol - it says initialArbOSState (here). Do you know where its getting the current block? 2, 3, 4 make sense to me! Thank you

zacshowa commented 1 month ago

1. okay maybe the naming is confusing lol - it says `initialArbOSState` ([here](https://github.com/EspressoSystems/nitro-espresso-integration/blob/integration/cmd/replay/main.go#L249)). Do you know where its getting the current block?
   2, 3, 4 make sense to me! Thank you
I think the naming might be confusing. Maybe by initialArbOSState the arbitrum devs meant "The ArbOS state before the STF finishes. implying it may be different when it ends. However, it should be the state based on the most recent block. In main.go, main() grabs the latest block height to get the state root and open the statedb here. It then uses this statedb to open ArbOS state here.

EspressoSystems / nitro-espresso-integration