-
Notifications
You must be signed in to change notification settings - Fork 928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: adds support for gRPC streaming and removes dependency on RPC #3915
base: feature/api-breaks
Are you sure you want to change the base?
Changes from 22 commits
e831ee3
ea9a2d7
5cd2e94
c30fdbc
9a194b2
8360531
2b71210
13bddc3
a568598
291952b
3087a3a
cc81362
7d89608
51096af
fe54137
62c1dc7
184391b
e11d333
b37c4b2
b14cb16
e51f237
221253f
fe20093
f371c33
92c4f94
af7f2be
7a62570
1b41b56
a863f6b
b5a545f
a10ac73
d4243fa
511068a
a033feb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we already know, Data is quite big, let's avoid copying it here. It should be easy to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will be GC at some point. The thing is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we passing by value here? Just curious There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
if err != nil { | ||
return nil, fmt.Errorf("extending block data for height %d: %w", b.Header.Height, err) | ||
} | ||
// create extended header | ||
eh, err := ce.construct(&b.Header, &b.Commit, &b.ValidatorSet, eds) | ||
eh, err := ce.construct(b.Header, b.Commit, b.ValidatorSet, eds) | ||
if err != nil { | ||
panic(fmt.Errorf("constructing extended header for height %d: %w", b.Header.Height, err)) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package core | |
import ( | ||
"bytes" | ||
"context" | ||
"net" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -62,6 +63,7 @@ func TestCoreExchange_RequestHeaders(t *testing.T) { | |
require.NoError(t, err) | ||
assert.True(t, has) | ||
} | ||
require.NoError(t, fetcher.Stop(ctx)) | ||
} | ||
|
||
// TestExchange_DoNotStoreHistoric tests that the CoreExchange will not | ||
|
@@ -118,7 +120,11 @@ func createCoreFetcher(t *testing.T, cfg *testnode.Config) (*BlockFetcher, testn | |
// flakiness with accessing account state) | ||
_, err := cctx.WaitForHeightWithTimeout(2, time.Second*2) // TODO @renaynay: configure? | ||
require.NoError(t, err) | ||
return NewBlockFetcher(cctx.Client), cctx | ||
host, port, err := net.SplitHostPort(cctx.GRPCClient.Target()) | ||
require.NoError(t, err) | ||
blockAPIClient, err := NewRemote(host, port) | ||
require.NoError(t, err) | ||
return NewBlockFetcher(blockAPIClient), cctx | ||
} | ||
|
||
// fillBlocks fills blocks until the context is canceled. | ||
|
@@ -136,7 +142,9 @@ func fillBlocks( | |
} | ||
|
||
_, err := cctx.FillBlock(16, cfg.Genesis.Accounts()[0].Name, flags.BroadcastBlock) | ||
require.NoError(t, err) | ||
if err != nil && ctx.Err() == nil { | ||
require.NoError(t, err) | ||
Comment on lines
+147
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just require.NoError? we do not expect one here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a funny behavior happening there that I guess will be fixed if we support closing the connection manually. Related: #3915 (comment) |
||
} | ||
} | ||
} | ||
|
||
|
@@ -154,7 +162,7 @@ func generateNonEmptyBlocks( | |
sub, err := fetcher.SubscribeNewBlockEvent(ctx) | ||
require.NoError(t, err) | ||
defer func() { | ||
err = fetcher.UnsubscribeNewBlockEvent(ctx) | ||
err = fetcher.Stop(ctx) | ||
require.NoError(t, err) | ||
}() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the client across
state
andcore
modules?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm planning to do it in a subsequent PR