feat!: adds support for gRPC streaming and removes dependency on RPC #3915

rach-id · 2024-11-04T13:45:43Z

The Celestia-node part of celestiaorg/celestia-core#1513

rach-id · 2024-11-04T13:48:13Z

PR still not fully ready, some tests are still failing. but would be good to get initial opinion + if someone can see if it's providing you with any performance increase

rach-id · 2024-11-04T13:52:58Z

core/fetcher.go

+	case <-f.doneCh:
+		return nil
+	case <-ctx.Done():
+		return fmt.Errorf("fetcher: unsubscribe from new block events: %w", ctx.Err())


[question for reviewers]
not sure if this is the right way to stop the resources in here. I guess we can delete this part. What do you think?

I think it's worth keeping. It might look unnecessary, but it will actually prevent flakes for tests, so lets keep it.

what about closing the underlying connection? What's responsible for that?

good catch. I can support closing the connection if we make the Client a struct. would that be something we want?

rach-id · 2024-11-05T09:22:08Z

nodebuilder/core/module.go

-				fx.OnStart(func(_ context.Context, client core.Client) error {
-					return client.Start()
-				}),
-				fx.OnStop(func(_ context.Context, client core.Client) error {
-					return client.Stop()
-				}),


[note for reviewers]
with the new implementation, the gRPC connection is started when creating the client instead of waiting until we call Start()

Yes but we also need to close conn when node stops.

#3915 (comment)

go.mod

rach-id · 2024-11-05T09:25:35Z

go.mod

 	github.com/filecoin-project/dagstore => github.com/celestiaorg/dagstore v0.0.0-20230824094345-537c012aa403
 	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
 	// broken goleveldb needs to be replaced for the cosmos-sdk and celestia-app
 	github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
-	github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.43.0-tm-v0.34.35
+	github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.43.0-tm-v0.34.35.0.20241104132041-3d2b32b7ddf2


[note for reviewers]
will be updated once we merge the PRs and make release

the same here

codecov-commenter · 2024-11-05T09:31:10Z

Codecov Report

Attention: Patch coverage is 54.23077% with 119 lines in your changes missing coverage. Please review.

Project coverage is 45.24%. Comparing base (2469e7a) to head (f59ca34).
Report is 393 commits behind head on main.

Files with missing lines	Patch %	Lines
core/fetcher.go	51.32%	62 Missing and 30 partials ⚠️
core/client.go	54.54%	13 Missing and 2 partials ⚠️
core/exchange.go	71.42%	2 Missing ⚠️
core/listener.go	33.33%	2 Missing ⚠️
nodebuilder/core/config.go	50.00%	2 Missing ⚠️
nodebuilder/core/constructors.go	0.00%	2 Missing ⚠️
nodebuilder/core/module.go	0.00%	2 Missing ⚠️
nodebuilder/core/opts.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3915      +/-   ##
==========================================
+ Coverage   44.83%   45.24%   +0.40%     
==========================================
  Files         265      308      +43     
  Lines       14620    21997    +7377     
==========================================
+ Hits         6555     9952    +3397     
- Misses       7313    10953    +3640     
- Partials      752     1092     +340

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nodebuilder/core/config.go

vgonkivs · 2024-11-13T11:24:03Z

Tested locally. Both grpc clients work

cristaloleg · 2024-11-19T09:57:49Z

state/core_access.go

@@ -123,7 +123,7 @@ func (ca *CoreAccessor) Start(ctx context.Context) error {
 	ca.ctx, ca.cancel = context.WithCancel(context.Background())

 	// dial given celestia-core endpoint
-	endpoint := fmt.Sprintf("%s:%s", ca.coreIP, ca.grpcPort)
+	endpoint := fmt.Sprintf("%s:%s", ca.coreIP, ca.port)


Nit: https://pkg.go.dev/net#JoinHostPort

Wondertan · 2024-11-08T00:28:50Z

core/fetcher.go

+
+// resolveHeight takes a height pointer and returns its value if it's not nil.
+// otherwise, returns the latest height.
+func (f *BlockFetcher) resolveHeight(ctx context.Context, height *int64) (int64, error) {


[non-blocking]
Passing a pointer for an int is really an anti-pattern, which existed in the previous client. I guess you kept it for the case where the nil is passed which means get the latest, but we don't need to stick to that pattern, and can use something else, like a separate method.

Yes, I tried to keep the same API. I don't like passing pointers to numbers too. Do you think this should be updated in this PR? or we create an issue and leave it later on for someone to pick it up at some point?

Wondertan · 2024-11-20T10:31:12Z

core/fetcher.go

+	case <-f.doneCh:
+		return nil
+	case <-ctx.Done():
+		return fmt.Errorf("fetcher: unsubscribe from new block events: %w", ctx.Err())


I think it's worth keeping. It might look unnecessary, but it will actually prevent flakes for tests, so lets keep it.

Wondertan · 2024-11-20T10:33:39Z

core/exchange.go

@@ -173,12 +173,12 @@ func (ce *Exchange) getExtendedHeaderByHeight(ctx context.Context, height *int64
 	}
 	log.Debugw("fetched signed block from core", "height", b.Header.Height)

-	eds, err := extendBlock(b.Data, b.Header.Version.App)
+	eds, err := extendBlock(*b.Data, b.Header.Version.App)


As we already know, Data is quite big, let's avoid copying it here. It should be easy to change extendBlock to take ptr instead.

this will be GC at some point. The thing is extendBlock also makes a couple other calls by value instead of reference. So multiple copies will be done regardless, this will be just one less. What do you think of creating an issue that checks wherever Data is used, it passes it by reference, even in app and maybe other repos so that we have everything fixed?

Wondertan · 2024-11-20T10:56:00Z

core/fetcher.go

-	if !f.client.IsRunning() {
-		return nil, errors.New("client not running")
-	}
-
 	ctx, cancel := context.WithCancel(ctx)
 	f.cancel = cancel
 	f.doneCh = make(chan struct{})


I recommend fool-proving this a bit. Basically, the current logic supports only a single subscription, while the method makes it look like multiple are and calling it multiple times is a bug.

I kept the same existing pattern. What do you mean by fool-proving?

I meant making erroring or panicking if subsvription is called again

d4243fa like this?

Wondertan · 2024-11-20T11:00:09Z

core/fetcher.go

+	if height != nil {
+		return *height, nil
+	}
+	status, err := f.client.Status(ctx, &coregrpc.StatusRequest{})


Isn't there a way to request the latest height through existing methods? Let's avoid additional roundtrip for Head request if possible.

Like 0 on BlockByHeightRequest can represent latest height.

That's why I recommended to just use 0

Im fine with changing and removing the pointers. Is this something you want to do in this PR?

Yes pls, we would like to leave as minimal tech debt behind as possiblr

core/testing.go

Wondertan · 2024-11-20T11:06:25Z

core/client.go

-		"/websocket",
-		httpClient.StandardClient(),
-	)
+	return coregrpc.StartBlockAPIGRPCClient(fmt.Sprintf("tcp://%s:%s", ip, port))


Can we reuse the client across state and core modules?

Yes. I'm planning to do it in a subsequent PR

walldiss · 2024-11-20T11:31:08Z

core/fetcher.go

+func receiveBlockByHeight(streamer coregrpc.BlockAPI_BlockByHeightClient) (
+	*types.Block,
+	*types.BlockMeta,
+	*types.Commit,
+	*types.ValidatorSet,
+	error,
+) {


Return of 5 values caught my eye. I can't find if BlockMeta returned from this method is used anywhere. If not, perhaps we can return just SignedBlock struct, which contains everything except Blockmeta?

I am returning everything we receive in this. If you don't want block meta to be returned, I guess we could even stop sending it from core. What do you think?

walldiss · 2024-11-20T11:37:00Z

core/fetcher.go

+			signedBlock, err := f.GetSignedBlock(ctx, &resp.Height)
+			if err != nil {
+				log.Errorw("fetcher: error receiving signed block", "height", resp.Height, "err", err.Error())
+				return
+			}


Lets add per-call timeout. Otherwise we might get stuck on one of the calls indefinitely

Also would need to handle canceled context here

renaynay

Many thanks for your efforts here @rach-id !

This PR made me think about #3952 which would allow us to only rely on GetSignedBlock endpoint in fetcher and remove all the other unnecessary fluff (like GetBlockInfo etc.

renaynay · 2024-11-20T10:29:01Z

core/fetcher.go

+
+// resolveHeight takes a height pointer and returns its value if it's not nil.
+// otherwise, returns the latest height.
+func (f *BlockFetcher) resolveHeight(ctx context.Context, height *int64) (int64, error) {


[nit]: Wouldn't this be a bit more simple if we just used 0 height to indicate Head instead of nil?

#3915 (comment)

renaynay · 2024-11-20T11:01:44Z

core/exchange.go

@@ -173,12 +173,12 @@ func (ce *Exchange) getExtendedHeaderByHeight(ctx context.Context, height *int64
 	}
 	log.Debugw("fetched signed block from core", "height", b.Header.Height)

-	eds, err := extendBlock(b.Data, b.Header.Version.App)
+	eds, err := extendBlock(*b.Data, b.Header.Version.App)


Why are we passing by value here? Just curious

because extendBlock expects the parameter to be passed by value. I didn't want to change that behavior since Data is already being copied in a lot of places, one more copy is fine. Related: #3915 (comment)

renaynay · 2024-11-20T11:54:32Z

core/exchange_test.go

+		if err != nil && ctx.Err() == nil {
+			require.NoError(t, err)


Why not just require.NoError? we do not expect one here

there is a funny behavior happening there that I guess will be fixed if we support closing the connection manually. Related: #3915 (comment)

renaynay · 2024-11-20T12:01:18Z

core/fetcher.go

+	case <-f.doneCh:
+		return nil
+	case <-ctx.Done():
+		return fmt.Errorf("fetcher: unsubscribe from new block events: %w", ctx.Err())


what about closing the underlying connection? What's responsible for that?

renaynay · 2024-11-20T12:53:24Z

nodebuilder/core/module.go

-				fx.OnStart(func(_ context.Context, client core.Client) error {
-					return client.Start()
-				}),
-				fx.OnStop(func(_ context.Context, client core.Client) error {
-					return client.Stop()
-				}),


Yes but we also need to close conn when node stops.

The base branch was changed.

# Conflicts: # core/exchange.go # core/exchange_test.go

rach-id added 8 commits November 2, 2024 23:24

feat!: use blocks streaming API instead of RPC in fetcher

e831ee3

test: node builder test using the new grpc endpoints

ea9a2d7

chore: import

5cd2e94

chore: add stop function for fetcher

c30fdbc

feat!: remove rpc port

9a194b2

chore: remove core rpc tests

8360531

chore: remove core rpc tests

2b71210

fix: test fill blocks failing after node stop

13bddc3

rach-id self-assigned this Nov 4, 2024

rach-id requested review from renaynay, Wondertan, vgonkivs, walldiss and cristaloleg as code owners November 4, 2024 13:45

github-actions bot added the external Issues created by non node team members label Nov 4, 2024

chore: use commits for deps

a568598

rach-id commented Nov 4, 2024

View reviewed changes

chore: fmt

291952b

rach-id marked this pull request as draft November 4, 2024 13:56

chore: fix tests

3087a3a

rach-id commented Nov 5, 2024

View reviewed changes

rach-id added 2 commits November 5, 2024 13:22

chore: fmt

cc81362

chore: fmt

7d89608

rach-id commented Nov 5, 2024

View reviewed changes

go.mod Show resolved Hide resolved

rach-id commented Nov 5, 2024

View reviewed changes

rach-id marked this pull request as ready for review November 5, 2024 09:36

rach-id added 2 commits November 7, 2024 01:12

chore: memory optimisation

51096af

chore: rename to first part

fe54137

rach-id changed the base branch from main to feature/api-breaks November 12, 2024 07:06

rach-id added 2 commits November 12, 2024 11:09

chore: remove unnecessary =nil

e51f237

chore: fix test

221253f

vgonkivs added kind:break! Attached to breaking PRs kind:feat Attached to feature PRs labels Nov 12, 2024

vgonkivs reviewed Nov 12, 2024

View reviewed changes

nodebuilder/core/config.go Outdated Show resolved Hide resolved

chore: rename grpc port to port

fe20093

vgonkivs previously approved these changes Nov 13, 2024

View reviewed changes

fix(header)!: tendermint compatible json marshall (celestiaorg#3928)

4a90605

cristaloleg reviewed Nov 19, 2024

View reviewed changes

Wondertan reviewed Nov 20, 2024

View reviewed changes

walldiss reviewed Nov 20, 2024

View reviewed changes

renaynay reviewed Nov 20, 2024

View reviewed changes

rach-id added 11 commits November 20, 2024 21:01

chore: use net.JoinHostPort as suggested by @cristaloleg

f371c33

chore: context timeout as suggested by @walldiss

92c4f94

chore: remove unnecessary comment

af7f2be

feat!: remove pointers in requests

7a62570

Merge branch 'feature/api-breaks' into support-grpc-endpoints

1b41b56

chore: bump version

a863f6b

chore: fix remaining implementation to support removing pointers

b5a545f

chore: lint

a10ac73

chore: check if fetcher is listening for new blocks

d4243fa

fix: correctly set the is listening for blocks to false

511068a

feat: support stopping the gRPC connection + subscription retry

a033feb

rach-id changed the base branch from feature/api-breaks to main November 25, 2024 14:48

rach-id added 3 commits November 25, 2024 15:50

Merge branch 'main' into support-grpc-endpoints

c1970f0

# Conflicts: # core/exchange.go # core/exchange_test.go

chore: remove unnecessary import

b640490

Merge branch 'main' into support-grpc-endpoints

f59ca34

feat!: adds support for gRPC streaming and removes dependency on RPC #3915

Are you sure you want to change the base?

feat!: adds support for gRPC streaming and removes dependency on RPC #3915

Conversation

rach-id commented Nov 4, 2024

rach-id commented Nov 4, 2024

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

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

Choose a reason for hiding this comment

codecov-commenter commented Nov 5, 2024 • edited Loading

Codecov Report

vgonkivs commented Nov 13, 2024 • 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

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

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

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

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

renaynay left a 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

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

Choose a reason for hiding this comment

codecov-commenter commented Nov 5, 2024 •

edited

Loading

vgonkivs commented Nov 13, 2024 •

edited

Loading