New DALC API by tzdybal · Pull Request #341 · evstack/ev-node

tzdybal · 2022-03-25T11:01:31Z

This PR introduces new DALC API into optimint.

Most important changes:

BlockManager needs to keep track of DA layer height
single DA layer block can contain multiple Optimint blocks
sync is now more complicated - but I tried to keep it as simple as possible

Actually, there's no need to explicitly map DA layer height to Optimint height.

Resolves #281.

codecov-commenter · 2022-03-25T11:05:40Z

Codecov Report

Merging #341 (7dae373) into main (404e426) will increase coverage by 0.50%.
The diff coverage is 77.81%.

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   58.61%   59.12%   +0.50%     
==========================================
  Files          44       44              
  Lines        7092     7180      +88     
==========================================
+ Hits         4157     4245      +88     
- Misses       2363     2364       +1     
+ Partials      572      571       -1

Impacted Files	Coverage Δ
config/config.go	`84.21% <ø> (ø)`
types/pb/optimint/optimint.pb.go	`43.99% <ø> (ø)`
types/pb/dalc/dalc.pb.go	`45.18% <62.26%> (+0.36%)`	⬆️
da/grpc/mockserv/mockserv.go	`82.35% <83.33%> (+1.50%)`	⬆️
da/grpc/grpc.go	`73.13% <84.00%> (+5.27%)`	⬆️
da/mock/mock.go	`79.54% <84.21%> (+13.97%)`	⬆️
block/manager.go	`64.11% <91.37%> (+4.58%)`	⬆️
log/test/loggers.go	`84.61% <100.00%> (+3.66%)`	⬆️
state/state.go	`92.10% <100.00%> (+0.43%)`	⬆️
store/store.go	`68.05% <100.00%> (-1.49%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 404e426...7dae373. Read the comment docs.

jbowen93

This LGTM. I've run the simple ethermint RPC tests against the CI built evmos image using the ephemeral cluster and they pass.

I'm confused how the tests are passing since the ephemeral cluster setup using an older DALC image which shouldn't have a compatible API...

tzdybal · 2022-03-29T09:27:21Z

I'm confused how the tests are passing since the ephemeral cluster setup using an older DALC image which shouldn't have a compatible API...

IIRC in ephemeral cluster we only have single optimint node, which is an aggregator. It shouldn't sync, so it's not using changed API. Only change in part related to block submission was adding single field in DAResult - this will just have zero-value, and this shouldn't be crucial for Optimint operation.

tzdybal · 2022-04-07T16:39:54Z

Pinging all approvors (@liamsi @jbowen93 @adlerjohn @evan-forbes) ;) PR is updated. All comments addressed.

This mock DA implementation reflects all new requirements: - use new DA API (DA height oriented) - store multiple Optimint block in single DA block (at same height) - optimint blocks may be located in non-consecutive DA blocks New mock requires Optimint to implement block height mapping, improved sync logic. Many unit tests are going to fail now. Implementing mentioned features is required to fix the tests.

This time mock mimicks actual DA layer completely. It "creates blocks" at given intervals. Block height can be incremented by more than 1.

With new DALC API, syncing logic has to be changed. Block manager has to keep track of DA height (it's also saved in state).

adlerjohn · 2022-04-07T20:19:46Z

+					m.logger.Error("failed to retrieve block from DALC", "daHeight", daHeight, "errors", err.Error())
+					break
+				}
+				atomic.AddUint64(&m.daHeight, 1)


Wait, are you sure this is the right use of atomics?

What if &m.daHeight changes in between loading it and adding 1 to it here? Then you'd be adding 2 to it.

This is the only place where m.daHeight is changed. It's atomic change, because we're reading this variable in several other places (in other goroutines). TBH the LoadUint64 in the beginning of the loop is not necessary (normal read should be fine), but I decided to handle this variable "atomically" everywhere for consistency.

My concern is if e.g. &m.daHeight is N, then it could be read as N in two separate threads and incremented twice to N+2, completely bypassing N+1. Is that ever possible, if is so, is it a problem?

It's not possible right now, as there is only one thread incrementing this variable, and I don't see a reason to add more threads/goroutines.

adlerjohn

Not going to block on #341 (comment), but if it's an issue can you open an PR to further investigate.

tzdybal requested review from adlerjohn and liamsi as code owners March 25, 2022 11:01

tzdybal requested review from Manav-Aggarwal, evan-forbes and jbowen93 March 25, 2022 11:07

tzdybal self-assigned this Mar 25, 2022

liamsi reviewed Mar 25, 2022

View reviewed changes

Comment thread block/manager.go Outdated

Comment thread block/manager.go

Comment thread block/manager.go Outdated

Comment thread block/manager.go

Comment thread config/config.go Outdated

jbowen93 reviewed Mar 28, 2022

View reviewed changes

Comment thread block/manager.go Outdated

Comment thread block/manager.go Outdated

tzdybal mentioned this pull request Apr 7, 2022

Add exponential back-off for DA retries. #344

Closed

tzdybal added 12 commits April 7, 2022 18:40

feat: new DALC proto definitions

1879977

feat: implement new DALC API

4db8cc2

refactor: ResultRetrieveBlock->ResultRetrieveBlocks

695966a

feat: improve mock DA implementation

f2395d7

This time mock mimicks actual DA layer completely. It "creates blocks" at given intervals. Block height can be incremented by more than 1.

feature: improve GRPC and mockserv DA implementations

13d3e68

feature: adopt DA tests to new DA layer mock

55d20bb

feature: implement new block sync logic

d972d1f

With new DALC API, syncing logic has to be changed. Block manager has to keep track of DA height (it's also saved in state).

fix: goimports files

5ff1ddd

refactor: use sync.Cond for retrieval signalling

1902905

fix: make test logger actually threadsafe

8a758cb

fix: address review comments

eb2e87c

tzdybal force-pushed the tzdybal/new_dalc_api branch from 2be9218 to eb2e87c Compare April 7, 2022 16:40

adlerjohn reviewed Apr 7, 2022

View reviewed changes

tzdybal mentioned this pull request Apr 8, 2022

Rename data_layer_height to da_height in DALC proto defitnitions #345

Closed

refactor: rename DataLayerHeight to DAHeight

7dae373

tzdybal requested review from adlerjohn and liamsi April 8, 2022 08:38

feat: add DAStartHeight configuration to block.Manager

3e4a966

adlerjohn approved these changes Apr 10, 2022

View reviewed changes

tzdybal merged commit cf71b4d into main Apr 10, 2022

tzdybal deleted the tzdybal/new_dalc_api branch April 10, 2022 08:51

tzdybal mentioned this pull request Jul 27, 2022

Handle multiple potential blocks per data availability layer block #277

Closed

Conversation

tzdybal commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbowen93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tzdybal commented Mar 29, 2022

Uh oh!

tzdybal commented Apr 7, 2022

Uh oh!

adlerjohn Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

tzdybal Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

adlerjohn Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

tzdybal Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adlerjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tzdybal commented Mar 25, 2022 •

edited

Loading

codecov-commenter commented Mar 25, 2022 •

edited

Loading