Use IPLD Dag instead of CoreAPI #352

Wondertan · 2021-05-23T20:31:53Z

The PR's main goal is to facilitate everything related to IPFS usage by:

Using IPLD Merkle DAG interfaces instead of CoreAPI.
Reworking Data retrieval logic to be based solely on IPLD Dag(optimization)
Use of Dag Sessions(optimization)
Reworking and cleaning some tests

DAS timings before 2 above optimizations:

I[2021-05-23|23:12:11.531] Successfully finished DAS sampling           height=629 numSamples=15 elapsedtime=1.015415079s
I[2021-05-23|23:12:11.768] Starting Data Availability sampling          height=630 numSamples=15 squareWidth=16
I[2021-05-23|23:12:12.690] Successfully finished DAS sampling           height=630 numSamples=15 elapsedtime=921.977083ms
I[2021-05-23|23:12:13.060] Starting Data Availability sampling          height=631 numSamples=15 squareWidth=16
I[2021-05-23|23:12:14.801] Successfully finished DAS sampling           height=631 numSamples=15 elapsedtime=1.740647283s
I[2021-05-23|23:12:15.003] Starting Data Availability sampling          height=632 numSamples=15 squareWidth=16

DAS timings after optimizations:

I[2021-05-23|23:13:44.353] Successfully finished DAS sampling           height=648 numSamples=15 elapsedtime=239.112542ms
I[2021-05-23|23:13:44.485] Starting Data Availability sampling          height=649 numSamples=15 squareWidth=16
I[2021-05-23|23:13:44.719] Successfully finished DAS sampling           height=649 numSamples=15 elapsedtime=234.11647ms
I[2021-05-23|23:13:44.853] Starting Data Availability sampling          height=650 numSamples=15 squareWidth=16
I[2021-05-23|23:13:45.095] Successfully finished DAS sampling           height=650 numSamples=15 elapsedtime=242.104799ms
I[2021-05-23|23:13:45.227] Starting Data Availability sampling          height=651 numSamples=15 squareWidth=16

NOTE: Heights in the run with optimization was not DASed before, so light-client didn't have common data before.

codecov-commenter · 2021-05-23T21:07:36Z

Codecov Report

❗ No coverage uploaded for pull request base (ismail/light-mvp@dbea20c). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##             ismail/light-mvp     #352   +/-   ##
===================================================
  Coverage                    ?   61.85%           
===================================================
  Files                       ?      262           
  Lines                       ?    22930           
  Branches                    ?        0           
===================================================
  Hits                        ?    14184           
  Misses                      ?     7251           
  Partials                    ?     1495

evan-forbes · 2021-05-24T01:31:07Z

p2p/ipld/read.go

+	total /= 2
+	if leaf < total {
+		root = lnks[0].Cid
+	} else {
+		root, leaf = lnks[1].Cid, leaf-total
+	}


mind adding some documentation somewhere? doesn't have to be anything too elaborate, but I think some simple guidance would make a big difference. Particularly for readers who are not familiar with our plugin.

Sure. Also, that’s no the final form of the PR. I think I will close ot and rewrite afresh basing on the master, but that requires #323 to be merged first.

I added some documentation now, hope it will help understand the logic

Cool! Just saw that after reviewing.

Use of Dag Sessions(optimization)

I think I understand this now. Although it's not clear to me how it works under the hood. But it makes sense intuitively.

Wondertan · 2021-05-26T19:43:45Z

@liamsi, @evan-forbes, I decided not to close this PR and instead extend it with the requested changes for CoreAPI removal

Wondertan · 2021-05-26T19:47:11Z

In follow-up PR I will go even further and will propose our custom interface for network DataAvailavility which works over DAG

liamsi

This looks great! Left some suggestions and questions.

liamsi · 2021-05-26T20:05:19Z

consensus/byzantine_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 	"time"

+	mdutils "github.com/ipfs/go-merkledag/test"


Can we use another alias? mdutils sounds like markdown utils. Suggestion: dagutils? dagtest?

That's a default package name.

I won't change that as that's actually temporary thing

Later in block propagation PR instead of just Mock I will use Mock with Mock networking

So I won't spend time for this renaming

That's a default package name.

But what does that even mean? Is the package aliased like this in other contexts?

light/client.go

p2p/ipld/read.go

p2p/ipld/read_test.go

evan-forbes · 2021-05-26T20:17:52Z

p2p/ipld/read.go

+// GetLeafData fetches and returns the raw leaf.
+// It walks down the IPLD NMT tree until it finds the requested one.
+func GetLeaf(ctx context.Context, dag format.NodeGetter, root cid.Cid, leaf, total uint32) (format.Node, error) {
+	// request the node
+	nd, err := dag.Get(ctx, root)


the new GetLeaf is much simpler! I like it a lot 👍

@liamsi do we need to update ADR002 with this slight api change?

Yeah, it would be good if the ADR would match the implementation's APIs.

I think so, but also I am going to propose an elegant PR soon. If it's get accepted, updating this will make more sense

liamsi · 2021-05-26T20:30:30Z

(from CI)

race: limit on 8128 simultaneously alive goroutines is exceeded, dying
FAIL github.com/lazyledger/lazyledger-core/p2p/ipld 16.744s

This is why the race detector hack was there btw.

Wondertan · 2021-05-26T20:33:46Z

@liamsi, I guessed that removing Mock IPFS will decrease the number of goroutines to remove the hack. WIll try something else to fix that

liamsi · 2021-05-26T20:48:49Z

p2p/ipld/read_test.go

 		t.Run(fmt.Sprintf("%s size %d", tc.name, tc.squareSize), func(t *testing.T) {
-			// if we're using the race detector, skip some large tests due to time and
-			// concurrency constraints
-			if racedetector.IsActive() && tc.squareSize > 8 {


To save you some time and to unblock this PR: we can either revert the changes regarding the racedetector, or, we simply skip all tests with a squareSize > 8.

evan-forbes

two 👍 from me! all of these refactors and optimizations are really adding up! ✨

Wondertan · 2021-05-26T20:50:53Z

@liamsi, @evan-forbes, after looking deeply into the reasons why the issue above happens, I conclude that we have a goroutine leak that we should fix. With that hack, we just solely ignored the issue, so I don't think taking the hack back makes sense.

We must limit the amount of spawned:

go sc.retrieveShare(rootCid, true, row, col, dag)

Wondertan · 2021-05-26T20:53:10Z

I would like to handle this myself as part of this PR, but I am really tired to do that today and it is midnight my time

evan-forbes · 2021-05-26T20:54:07Z

we don't have to merge this tonight. I'll try limiting the goroutines on a separate branch and see what happens

Wondertan · 2021-05-26T20:55:42Z

Ok, but I already pushed temporary hack to unblock this

liamsi · 2021-05-26T20:58:08Z

While it makes perfect sense to limit that number of go-routines spawned in that method, it's not a go-routine leak. Also note that the error only kicks in when the race-detector is active and the code works fine even with larger blocks without the race detector though.

I think, we can skip that test for now for larger blocks with a note that we should limit the number of go-routines spawned (I'll open an issue). If possible, let's merge this and handle the issue in a separate PR.

p2p/ipld/read_test.go

Wondertan · 2021-05-26T21:06:45Z

it's not a go-routine leak

That's true! Goruitne leak is when goroutine hangs out infinitely without returning, e.g. not calling cancel() for context will introduce one.

My original intuition was that spawning tonnes of routines are like leaking something. Obviously, it's wrong, affects performance, and should be avoided. Idk how to call that properly btw.

liamsi

Awesome work! 👍🏼 🚀

by moving the `codecov.yml` file from .github the root folder. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit c906604) Co-authored-by: Lasaro <lasaro@informal.systems>

Wondertan self-assigned this May 23, 2021

Wondertan requested review from evan-forbes, liamsi and tac0turtle as code owners May 23, 2021 20:31

evan-forbes reviewed May 24, 2021

View reviewed changes

Base automatically changed from ismail/light-mvp to master May 24, 2021 09:55

This comment has been minimized.

Sign in to view

feat(ipld): new GetLeafData with DAG session

66428c8

Wondertan force-pushed the hlib/mvp-experiments branch from 96460c9 to 2b648fe Compare May 26, 2021 19:19

Wondertan changed the title ~~Rework and optimize GetLeafData for Light Client~~ Use IPLD Dag instead of CoreAPI May 26, 2021

Wondertan force-pushed the hlib/mvp-experiments branch from 2b648fe to e87e122 Compare May 26, 2021 19:41

Wondertan requested a review from evan-forbes May 26, 2021 19:44

Wondertan force-pushed the hlib/mvp-experiments branch 2 times, most recently from 08e51ea to 631ab19 Compare May 26, 2021 19:58

refactor: use IPLD DAG instead whole CoreAPI

0d0ab65

Wondertan force-pushed the hlib/mvp-experiments branch from 631ab19 to 0d0ab65 Compare May 26, 2021 20:10

liamsi reviewed May 26, 2021

View reviewed changes

remove racedetector package

ad1d58b

evan-forbes reviewed May 26, 2021

View reviewed changes

update import aliases to be more dscriptive

660cac0

liamsi reviewed May 26, 2021

View reviewed changes

evan-forbes approved these changes May 26, 2021

View reviewed changes

temporary remove some tests

15f2162

liamsi reviewed May 26, 2021

View reviewed changes

p2p/ipld/read_test.go Show resolved Hide resolved

liamsi mentioned this pull request May 26, 2021

ipld: optionally limit the number of spawned go-rountines in RetrieveBlockData #357

Closed

liamsi approved these changes May 26, 2021

View reviewed changes

Wondertan merged commit f46cbc6 into master May 26, 2021

Wondertan deleted the hlib/mvp-experiments branch May 26, 2021 21:13

liamsi mentioned this pull request May 26, 2021

adr002 followups #308

Closed

evan-forbes mentioned this pull request May 27, 2021

Add the ipfs dag api object in Blockstore #356

Merged

Wondertan mentioned this pull request Dec 21, 2022

feat!: share.Getter and getters.IPLDGetter celestiaorg/celestia-node#1518

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use IPLD Dag instead of CoreAPI #352

Use IPLD Dag instead of CoreAPI #352

Wondertan commented May 23, 2021 •

edited

Loading

codecov-commenter commented May 23, 2021

evan-forbes May 24, 2021

Wondertan May 24, 2021 •

edited

Loading

liamsi May 24, 2021

Wondertan May 26, 2021

liamsi May 26, 2021

This comment has been minimized.

Wondertan commented May 26, 2021

Wondertan commented May 26, 2021

liamsi left a comment

liamsi May 26, 2021 •

edited

Loading

Wondertan May 26, 2021

Wondertan May 26, 2021

Wondertan May 26, 2021

Wondertan May 26, 2021

liamsi May 26, 2021 •

edited

Loading

evan-forbes May 26, 2021

evan-forbes May 26, 2021

liamsi May 26, 2021

Wondertan May 26, 2021

liamsi commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021 •

edited

Loading

liamsi May 26, 2021

evan-forbes left a comment

Wondertan commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021

evan-forbes commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021

liamsi commented May 26, 2021

Wondertan commented May 26, 2021 •

edited

Loading

liamsi left a comment

Use IPLD Dag instead of CoreAPI #352

Use IPLD Dag instead of CoreAPI #352

Conversation

Wondertan commented May 23, 2021 • edited Loading

codecov-commenter commented May 23, 2021

Codecov Report

Choose a reason for hiding this comment

Wondertan May 24, 2021 • 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

This comment has been minimized.

Wondertan commented May 26, 2021

Wondertan commented May 26, 2021

liamsi left a comment

Choose a reason for hiding this comment

liamsi May 26, 2021 • 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

liamsi May 26, 2021 • 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

liamsi commented May 26, 2021 • edited Loading

Wondertan commented May 26, 2021 • edited Loading

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Wondertan commented May 26, 2021 • edited Loading

Wondertan commented May 26, 2021

evan-forbes commented May 26, 2021 • edited Loading

Wondertan commented May 26, 2021

liamsi commented May 26, 2021

Wondertan commented May 26, 2021 • edited Loading

liamsi left a comment

Choose a reason for hiding this comment

Wondertan commented May 23, 2021 •

edited

Loading

Wondertan May 24, 2021 •

edited

Loading

liamsi May 26, 2021 •

edited

Loading

liamsi May 26, 2021 •

edited

Loading

liamsi commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021 •

edited

Loading

evan-forbes commented May 26, 2021 •

edited

Loading

Wondertan commented May 26, 2021 •

edited

Loading