Refactor openInitializeChainDb for Execution/Consensus split

[bragaigor]

Today openInitializeChainDb has procedures related to both Execution and Consensus components. So we split them into target functions related specifically to either Execution and Consensus.

Now openInitializeChainDb can be thought of as:

openInitializeChainDb(...)
|
├─> executionDB := openExistingExecutionDB(...) (executionDB, wasmDB, chainConfig)
|
├─> If executionDB == nil
|   |
|   ├─> checkAndDownloadDB(...)
|   |
|   ├─> openDownloadedExecutionDB(...) (executionDB, wasmDB)
|   |
|   ├─> getConsensusParsedInitMsg(...)
|   |
|   ├─> getNewBlockchain(...)
|   |
|   └─> getAndValidateGenesisAssertion
|
├─> PruneExecutionDB(...)
|
├─> validateBlockChain(...)
|
└─> recreateMissingStates(...)
|
└─> rebuildLocalWasm(...) (executionDB, l2BlockChain)

closes NIT-4199
closes NIT-4268

[codecov]

Codecov Report

❌ Patch coverage is 3.04878% with 477 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.55%. Comparing base (893facb) to head (6738f16).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4169      +/-   ##
==========================================
- Coverage   32.69%   32.55%   -0.14%     
==========================================
  Files         476      483       +7     
  Lines       56785    57089     +304     
==========================================
+ Hits        18564    18586      +22     
- Misses      34986    35254     +268     
- Partials     3235     3249      +14     

[github-actions]

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

[diegoximenes]

How about checking if it is easy to extend a bold test so it can test getGenesisAssertionCreationInfo and validateGenesisAssertion?
If it is easy then the test can be implemented 🙂

[pmikolajczyk41]

throwing two secondary comments for now

[diegoximenes]

Focused on only reviewing the commits added after my last review.

Code is way better than it was 🙂

Requesting changes mainly due to the lack of tests related to validate genesis assertion procedure.
We can discuss out of GitHub on how to test this in this PR, or if changes related to it can be reverted and introduced/tested in a new PR.
Also added some minor nitpicks, and some extra tests that can be added.

[diegoximenes]

I didn't review tests this time

[diegoximenes]

Suggestion: In case you find it will take more time to implement validate genesis assertion tests, you can revert changes related to validate genesis assertion, and open another PR to only implement those changes and the tests.
This PR already makes good improvements to the codebase, and we can merge it sooner.

The test for validate genesis assertion, if code related to it is changed, is a requirement.
Reviews of this part of the codebase already spotted some issues, which indicate that it is a good idea to have tests related to it.
Also, I am not completely sure how much coverage this validate genesis assertion procedure has is release qualification tests.

[bragaigor]

Suggestion: In case you find it will take more time to implement validate genesis assertion tests, you can revert changes related to validate genesis assertion, and open another PR to only implement those changes and the tests.
This PR already makes good improvements to the codebase, and we can merge it sooner.

Chatted with @rauljordan and was able to jot down a concise test for genesis assertion. Such test can be found in system_tests/genesis_assertion_test.go and in there you'll find 2 tests:

• One that uses createCompleteTestNodeOnL1 (used by BoLD tests)
• And another that uses builder

I thought to keep both since each of them starts L1 chain with different configuration, but let me know if you prefer to keep just one of them.

[diegoximenes]

If it is not a bold chain then getGenesisAssertionCreationInfo is returning a nil err, with an empty assertion hash.
However, in this case, GetAndValidateGenesisAssertion shouldn't continue and call validateGenesisAssertion.
getGenesisAssertionCreationInfo returning a bool, indicating if it is a bold chain, can help with that.

If possible, a test related to that should be added.

[bragaigor]

Added but will still try to add a test for it as it didn't seem that straightforward to force getGenesisAssertionCreationInfo into returning a not bold error, which would require for the call to ChallengeGracePeriodBlocks return an error.

[diegoximenes]

Some tests here are not able to detect if chain config returned by GetInit comes from gethexec.TryReadStoredChainConfig or chaininfo.GetChainConfig.

Using TestGetInitSkipInitDataReader as an example, you can OpenInitializeExecutionDB providing a node config with chain ID different than 42161, and then call GetInit providing another chain ID.

WDYT?

[bragaigor]

That's a good point, and I don't think we're testing the case that chain config returned by GetInit comes from gethexec.TryReadStoredChainConfig so I added a test for that at the bottom.

[diegoximenes]

I saw that this exists in other bold tests, but I don't get why new values need to be assigned to ctx, cancelCtx here

[bragaigor]

That's true, we already defer ctx cancel at the very beginning. This feels redundant for this test at least. Maybe ctx changes for BoLD test but not here.

[diegoximenes]

have no leftover accounts

Not sure what you mean by leftover accounts here.

however, that does not apply to a test environment, which is why it is safe to remove any account from ArbInitData

This is not clear to me why 🤔, maybe I am missing some context about this.

[bragaigor]

That's probably my poor explanation. So when we call GetAndValidateGenesisAssertion, one of the parameters passed to validateGenesisAssertion is initDataReaderHasAccounts, which is coming from accountsReader.More() here.

accountsReader.More() calls into:

nitro/statetransfer/memdatareader.go

Lines 30 to 32 in 1ee4784

	func (f *FieldReader) More() bool {
	return f.count < f.length
	}

if we backtrack a bit inside GetAndValidateGenesisAssertion we convert initDataReader into accountsReader with:

accountsReader, err := initDataReader.GetAccountDataReader()

and that's where we set length to the length of accounts:

nitro/statetransfer/memdatareader.go

Lines 93 to 98 in 1ee4784

	func (r *MemoryInitDataReader) GetAccountDataReader() (AccountDataReader, error) {
	return &MemoryAccountDataReader{
	FieldReader: FieldReader{
	m: r,
	length: len(r.d.Accounts),
	},

so as long as there's one or more accounts in builder.L2Info.ArbInitData, then GetAndValidateGenesisAssertion will always fail with genesis assertion is null but there are accounts in the init data. And that's what I call leftover accounts because it's a test environment, meaning, if we were dealing with a real chain and we set ValidateGenesisAssertion to true then initDataReaderHasAccounts would have been false. It's still unclear to me why in the case of new chains that's a problem but according to @rauljordan it seems to be a legacy configuration. We could remove that flag but not sure if that would result in introducing some unforseen bug that's not that clear right now.

Does that make sense?

[diegoximenes]

Is "rollup" an appropriate name here?

This latter case that you mention is: an Arbitrum chain is running without the bold protocol, and then there is an upgrade to use the bold protocol, right?
If that is true, then we are already testing GetAndValidateGenesisAssertion for the first case, through release qualification tests against Sepolia/Arb1, right?

[bragaigor]

Is "rollup" an appropriate name here?

Renaming to chain

This latter case that you mention is: an Arbitrum chain is running without the bold protocol, and then there is an upgrade to use the bold protocol, right?

that is correct

If that is true, then we are already testing GetAndValidateGenesisAssertion for the first case, through release qualification tests against Sepolia/Arb1, right?

if ValidateGenesisAssertion is set to true then yes. But the only place I saw that being set to true was in InitConfigDefault here, unless that's coming from somewhere else?