Remove Geth Bindings From Prysm #11586
Conversation
| @@ -354,6 +355,24 @@ func (s *Service) ExecutionBlocksByHashes(ctx context.Context, hashes []common.H | |||
| return execBlks, nil | |||
| } | |||
|
|
|||
| func (s *Service) HeaderByHash(ctx context.Context, hash common.Hash) (*pb.ExecutionBlock, error) { | |||
| return blk, err | ||
| } | ||
|
|
||
| func (s *Service) HeaderByNumber(ctx context.Context, number *big.Int) (*pb.ExecutionBlock, error) { |
| } | ||
|
|
||
| func (*RPCClient) Close() {} | ||
|
|
||
| func (*RPCClient) CallContext(_ context.Context, _ interface{}, _ string, _ ...interface{}) error { | ||
| func (r *RPCClient) CallContext(ctx context.Context, obj interface{}, methodName string, args ...interface{}) error { |
|
I don't understand the rationale here. In any case, can we simplify this further? We discussed offline that we only need the hash, timestamp and block number. Some subset of the block. Similar to how LH does it https://github.com/sigp/lighthouse/blob/6d5a2b509fac7b6ffe693866f58ba49989f946d7/common/eth2/src/lighthouse.rs#L314 They have a subset of the block struct they are using. I imagine this is far less memory usage than a full eth1 block with everything in it. |
|
OK -- actually I remember why. The go-ethereum structs recompute the hash when you call |
| func (s *Service) HeaderByHash(ctx context.Context, hash common.Hash) (*types.HeaderInfo, error) { | ||
| var hdr *types.HeaderInfo | ||
| err := s.rpcClient.CallContext(ctx, &hdr, ExecutionBlockByHashMethod, hash, false /* no transactions */) | ||
| if err == nil && hdr == nil { | ||
| err = ethereum.NotFound | ||
| } | ||
| return hdr, err | ||
| } | ||
|
|
||
| // HeaderByNumber returns the relevant header details for the provided block number. | ||
| func (s *Service) HeaderByNumber(ctx context.Context, number *big.Int) (*types.HeaderInfo, error) { | ||
| var hdr *types.HeaderInfo | ||
| err := s.rpcClient.CallContext(ctx, &hdr, ExecutionBlockByNumberMethod, toBlockNumArg(number), false /* no transactions */) | ||
| if err == nil && hdr == nil { | ||
| err = ethereum.NotFound | ||
| } | ||
| return hdr, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we have any way to test these? Mock rpc client or something? The functionality for returning ethereum.NotFound is something worth testing, unless it is very difficult to do so.
There was a problem hiding this comment.
This should be possible, I can give it a try
| if err := s.headerCache.AddHeader(blk); err != nil { | ||
| return nil, err | ||
| } | ||
| info, err = types.HeaderToHeaderInfo(blk) | ||
| if err != nil { |
There was a problem hiding this comment.
Oh cool, we were already storing a subset of the block information in the caches.
beacon-chain/execution/service.go
Outdated
| // fetching eth1 data from the clients. | ||
| type RPCDataFetcher interface { | ||
| Close() | ||
| HeaderByNumber(ctx context.Context, number *big.Int) (*gethTypes.Header, error) | ||
| HeaderByHash(ctx context.Context, hash common.Hash) (*gethTypes.Header, error) | ||
| } |
There was a problem hiding this comment.
Delete this whole thing?
| @@ -82,7 +84,7 @@ func (g *goodNotifier) StateFeed() *event.Feed { | |||
|
|
|||
| type goodFetcher struct { | |||
| backend *backends.SimulatedBackend | |||
| blockNumMap map[uint64]*gethTypes.Header | |||
| blockNumMap map[uint64]*pb.ExecutionBlock | |||
There was a problem hiding this comment.
This one is different than the mock rpcClient?
// beacon-chain/execution/testing/mock_execution_chain.go
// RPCClient defines the mock rpc client.
type RPCClient struct {
Backend *backends.SimulatedBackend
BlockNumMap map[uint64]*types.HeaderInfo // not pb.ExecutionBlock
}Also, can you just use that mock RPCClient?
There was a problem hiding this comment.
We don't need the whole mock actually, just deleting it now
beacon-chain/execution/service.go
Outdated
| type RPCClientBad struct { | ||
| } | ||
|
|
||
| func (RPCClientBad) Close() {} | ||
| func (RPCClientBad) BatchCall([]gethRPC.BatchElem) error { | ||
| return errors.New("rpc client is not initialized") | ||
| } | ||
|
|
||
| func (RPCClientBad) CallContext(context.Context, interface{}, string, ...interface{}) error { | ||
| return ethereum.NotFound | ||
| } | ||
|
|
There was a problem hiding this comment.
This looks like test code?
What type of PR is this?
Execution Code Cleanup
What does this PR do? Why is it needed?
Post merge, prysm retrieves raw headers and blocks from geth without using go bindings from the go-ethereum repo. This PR extends this change across all our execution interfacing code so that we can remove some unnecessary interfaces along with supporting gnosis chain changes.
Which issues(s) does this PR fix?
N.A
Other notes for review