From df1a80169ac521a093fdcc911e96c1c7bd7b9d10 Mon Sep 17 00:00:00 2001 From: rupam-04 Date: Tue, 2 Jul 2024 20:22:03 +0530 Subject: [PATCH 1/4] fix: resolve the errors in ```GetLightClientUpdatesByRange``` function --- beacon-chain/rpc/eth/light-client/handlers.go | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/beacon-chain/rpc/eth/light-client/handlers.go b/beacon-chain/rpc/eth/light-client/handlers.go index 4ba524bbf692..129c8a802f33 100644 --- a/beacon-chain/rpc/eth/light-client/handlers.go +++ b/beacon-chain/rpc/eth/light-client/handlers.go @@ -130,42 +130,44 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R } firstSlotInPeriod := period * slotsPerPeriod - // Let's not use the first slot in the period, otherwise the attested header will be in previous period - firstSlotInPeriod++ - var state state.BeaconState var block interfaces.ReadOnlySignedBeaconBlock for slot := lastSlotInPeriod; slot >= firstSlotInPeriod; slot-- { state, err = s.Stater.StateBySlot(ctx, types.Slot(slot)) if err != nil { - continue + httputil.HandleError(w, "Could not get state: "+err.Error(), http.StatusInternalServerError) + return } // Get the block latestBlockHeader := state.LatestBlockHeader() latestStateRoot, err := state.HashTreeRoot(ctx) if err != nil { - continue + httputil.HandleError(w, "Could not get state root: "+err.Error(), http.StatusInternalServerError) + return } latestBlockHeader.StateRoot = latestStateRoot[:] blockRoot, err := latestBlockHeader.HashTreeRoot() if err != nil { - continue + httputil.HandleError(w, "Could not get block root: "+err.Error(), http.StatusInternalServerError) + return } block, err = s.Blocker.Block(ctx, blockRoot[:]) if err != nil || block == nil { - continue + httputil.HandleError(w, "Could not get block: "+err.Error(), http.StatusInternalServerError) + return } syncAggregate, err := block.Block().Body().SyncAggregate() if err != nil || syncAggregate == nil { - continue + httputil.HandleError(w, "Could not get sync aggregate: "+err.Error(), http.StatusInternalServerError) + return } - if syncAggregate.SyncCommitteeBits.Count()*3 < config.SyncCommitteeSize*2 { + if syncAggregate.SyncCommitteeBits.Count() < 1 { // Not enough votes - continue + httputil.HandleError(w, "Not enough votes", http.StatusNotFound) } break @@ -173,20 +175,20 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R if block == nil { // No valid block found for the period - continue + httputil.HandleError(w, "No valid block found for the period", http.StatusNotFound) } // Get attested state attestedRoot := block.Block().ParentRoot() attestedBlock, err := s.Blocker.Block(ctx, attestedRoot[:]) if err != nil || attestedBlock == nil { - continue + httputil.HandleError(w, "Could not get attested block: "+err.Error(), http.StatusInternalServerError) } attestedSlot := attestedBlock.Block().Slot() attestedState, err := s.Stater.StateBySlot(ctx, attestedSlot) if err != nil { - continue + httputil.HandleError(w, "Could not get attested state: "+err.Error(), http.StatusInternalServerError) } // Get finalized block From 52c4d712a1d1d3962616a56998b1b7c21528e0aa Mon Sep 17 00:00:00 2001 From: rupam-04 Date: Tue, 2 Jul 2024 20:45:22 +0530 Subject: [PATCH 2/4] added missing returns and replaced ```HandleError``` with continue when "not enough votes" --- beacon-chain/rpc/eth/light-client/handlers.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/beacon-chain/rpc/eth/light-client/handlers.go b/beacon-chain/rpc/eth/light-client/handlers.go index 129c8a802f33..17653face06e 100644 --- a/beacon-chain/rpc/eth/light-client/handlers.go +++ b/beacon-chain/rpc/eth/light-client/handlers.go @@ -167,7 +167,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R if syncAggregate.SyncCommitteeBits.Count() < 1 { // Not enough votes - httputil.HandleError(w, "Not enough votes", http.StatusNotFound) + continue } break @@ -176,6 +176,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R if block == nil { // No valid block found for the period httputil.HandleError(w, "No valid block found for the period", http.StatusNotFound) + return } // Get attested state @@ -183,12 +184,14 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R attestedBlock, err := s.Blocker.Block(ctx, attestedRoot[:]) if err != nil || attestedBlock == nil { httputil.HandleError(w, "Could not get attested block: "+err.Error(), http.StatusInternalServerError) + return } attestedSlot := attestedBlock.Block().Slot() attestedState, err := s.Stater.StateBySlot(ctx, attestedSlot) if err != nil { httputil.HandleError(w, "Could not get attested state: "+err.Error(), http.StatusInternalServerError) + return } // Get finalized block From 9bf7b4b98c42bad0bd4a613c97218a70119a04d2 Mon Sep 17 00:00:00 2001 From: rupam-04 Date: Wed, 3 Jul 2024 03:00:14 +0530 Subject: [PATCH 3/4] feat: added unit tests for ```GetLightClientUpdatesByRange``` --- .../rpc/eth/light-client/handlers_test.go | 200 +++++++++++++++++- 1 file changed, 199 insertions(+), 1 deletion(-) diff --git a/beacon-chain/rpc/eth/light-client/handlers_test.go b/beacon-chain/rpc/eth/light-client/handlers_test.go index aa65d9b29203..ba09c3b6f38c 100644 --- a/beacon-chain/rpc/eth/light-client/handlers_test.go +++ b/beacon-chain/rpc/eth/light-client/handlers_test.go @@ -78,7 +78,7 @@ func TestLightClientHandler_GetLightClientBootstrap(t *testing.T) { require.NotNil(t, resp.Data) } -func TestLightClientHandler_GetLightClientUpdatesByRange(t *testing.T) { +func TestLightClientHandler_GetLightClientUpdatesByRange_MultipleSyncCommitteeBits(t *testing.T) { helpers.ClearCache() ctx := context.Background() config := params.BeaconConfig() @@ -180,6 +180,204 @@ func TestLightClientHandler_GetLightClientUpdatesByRange(t *testing.T) { require.NotNil(t, resp) } +func TestLightClientHandler_GetLightClientUpdatesByRange_ZeroSyncCommitteeBits(t *testing.T) { + helpers.ClearCache() + ctx := context.Background() + config := params.BeaconConfig() + slot := primitives.Slot(config.AltairForkEpoch * primitives.Epoch(config.SlotsPerEpoch)).Add(1) + + attestedState, err := util.NewBeaconStateCapella() + require.NoError(t, err) + err = attestedState.SetSlot(slot.Sub(1)) + require.NoError(t, err) + + parent := util.NewBeaconBlockCapella() + parent.Block.Slot = slot.Sub(1) + + signedParent, err := blocks.NewSignedBeaconBlock(parent) + require.NoError(t, err) + + parentHeader, err := signedParent.Header() + require.NoError(t, err) + attestedHeader := parentHeader.Header + + err = attestedState.SetLatestBlockHeader(attestedHeader) + require.NoError(t, err) + attestedStateRoot, err := attestedState.HashTreeRoot(ctx) + require.NoError(t, err) + + // get a new signed block so the root is updated with the new state root + parent.Block.StateRoot = attestedStateRoot[:] + signedParent, err = blocks.NewSignedBeaconBlock(parent) + require.NoError(t, err) + + st, err := util.NewBeaconStateCapella() + require.NoError(t, err) + err = st.SetSlot(slot) + require.NoError(t, err) + + parentRoot, err := signedParent.Block().HashTreeRoot() + require.NoError(t, err) + + block := util.NewBeaconBlockCapella() + block.Block.Slot = slot + block.Block.ParentRoot = parentRoot[:] + + signedBlock, err := blocks.NewSignedBeaconBlock(block) + require.NoError(t, err) + + h, err := signedBlock.Header() + require.NoError(t, err) + + err = st.SetLatestBlockHeader(h.Header) + require.NoError(t, err) + stateRoot, err := st.HashTreeRoot(ctx) + require.NoError(t, err) + + // get a new signed block so the root is updated with the new state root + block.Block.StateRoot = stateRoot[:] + signedBlock, err = blocks.NewSignedBeaconBlock(block) + require.NoError(t, err) + + root, err := block.Block.HashTreeRoot() + require.NoError(t, err) + + mockBlocker := &testutil.MockBlocker{ + RootBlockMap: map[[32]byte]interfaces.ReadOnlySignedBeaconBlock{ + parentRoot: signedParent, + root: signedBlock, + }, + SlotBlockMap: map[primitives.Slot]interfaces.ReadOnlySignedBeaconBlock{ + slot.Sub(1): signedParent, + slot: signedBlock, + }, + } + mockChainService := &mock.ChainService{Optimistic: true, Slot: &slot, State: st} + s := &Server{ + Stater: &testutil.MockStater{StatesBySlot: map[primitives.Slot]state.BeaconState{ + slot.Sub(1): attestedState, + slot: st, + }}, + Blocker: mockBlocker, + HeadFetcher: mockChainService, + } + startPeriod := slot.Div(uint64(config.EpochsPerSyncCommitteePeriod)).Div(uint64(config.SlotsPerEpoch)) + url := fmt.Sprintf("http://foo.com/?count=1&start_period=%d", startPeriod) + request := httptest.NewRequest("GET", url, nil) + writer := httptest.NewRecorder() + writer.Body = &bytes.Buffer{} + + s.GetLightClientUpdatesByRange(writer, request) + + require.Equal(t, http.StatusOK, writer.Code) + var resp []structs.LightClientUpdateWithVersion + require.NoError(t, json.Unmarshal(writer.Body.Bytes(), &resp)) + require.Equal(t, 1, len(resp)) + require.Equal(t, "capella", resp[0].Version) + require.Equal(t, hexutil.Encode(attestedHeader.BodyRoot), resp[0].Data.AttestedHeader.BodyRoot) + require.NotNil(t, resp) +} + +func TestLightClientHandler_GetLightClientUpdatesByRange_OneSyncCommitteeBit(t *testing.T) { + helpers.ClearCache() + ctx := context.Background() + config := params.BeaconConfig() + slot := primitives.Slot(config.AltairForkEpoch * primitives.Epoch(config.SlotsPerEpoch)).Add(1) + + attestedState, err := util.NewBeaconStateCapella() + require.NoError(t, err) + err = attestedState.SetSlot(slot.Sub(1)) + require.NoError(t, err) + + parent := util.NewBeaconBlockCapella() + parent.Block.Slot = slot.Sub(1) + + signedParent, err := blocks.NewSignedBeaconBlock(parent) + require.NoError(t, err) + + parentHeader, err := signedParent.Header() + require.NoError(t, err) + attestedHeader := parentHeader.Header + + err = attestedState.SetLatestBlockHeader(attestedHeader) + require.NoError(t, err) + attestedStateRoot, err := attestedState.HashTreeRoot(ctx) + require.NoError(t, err) + + // get a new signed block so the root is updated with the new state root + parent.Block.StateRoot = attestedStateRoot[:] + signedParent, err = blocks.NewSignedBeaconBlock(parent) + require.NoError(t, err) + + st, err := util.NewBeaconStateCapella() + require.NoError(t, err) + err = st.SetSlot(slot) + require.NoError(t, err) + + parentRoot, err := signedParent.Block().HashTreeRoot() + require.NoError(t, err) + + block := util.NewBeaconBlockCapella() + block.Block.Slot = slot + block.Block.ParentRoot = parentRoot[:] + + block.Block.Body.SyncAggregate.SyncCommitteeBits.SetBitAt(0, true) + + signedBlock, err := blocks.NewSignedBeaconBlock(block) + require.NoError(t, err) + + h, err := signedBlock.Header() + require.NoError(t, err) + + err = st.SetLatestBlockHeader(h.Header) + require.NoError(t, err) + stateRoot, err := st.HashTreeRoot(ctx) + require.NoError(t, err) + + // get a new signed block so the root is updated with the new state root + block.Block.StateRoot = stateRoot[:] + signedBlock, err = blocks.NewSignedBeaconBlock(block) + require.NoError(t, err) + + root, err := block.Block.HashTreeRoot() + require.NoError(t, err) + + mockBlocker := &testutil.MockBlocker{ + RootBlockMap: map[[32]byte]interfaces.ReadOnlySignedBeaconBlock{ + parentRoot: signedParent, + root: signedBlock, + }, + SlotBlockMap: map[primitives.Slot]interfaces.ReadOnlySignedBeaconBlock{ + slot.Sub(1): signedParent, + slot: signedBlock, + }, + } + mockChainService := &mock.ChainService{Optimistic: true, Slot: &slot, State: st} + s := &Server{ + Stater: &testutil.MockStater{StatesBySlot: map[primitives.Slot]state.BeaconState{ + slot.Sub(1): attestedState, + slot: st, + }}, + Blocker: mockBlocker, + HeadFetcher: mockChainService, + } + startPeriod := slot.Div(uint64(config.EpochsPerSyncCommitteePeriod)).Div(uint64(config.SlotsPerEpoch)) + url := fmt.Sprintf("http://foo.com/?count=1&start_period=%d", startPeriod) + request := httptest.NewRequest("GET", url, nil) + writer := httptest.NewRecorder() + writer.Body = &bytes.Buffer{} + + s.GetLightClientUpdatesByRange(writer, request) + + require.Equal(t, http.StatusOK, writer.Code) + var resp []structs.LightClientUpdateWithVersion + require.NoError(t, json.Unmarshal(writer.Body.Bytes(), &resp)) + require.Equal(t, 1, len(resp)) + require.Equal(t, "capella", resp[0].Version) + require.Equal(t, hexutil.Encode(attestedHeader.BodyRoot), resp[0].Data.AttestedHeader.BodyRoot) + require.NotNil(t, resp) +} + func TestLightClientHandler_GetLightClientUpdatesByRange_TooBigInputCount(t *testing.T) { helpers.ClearCache() ctx := context.Background() From f6db4a3afba17e581d4ad6ae9ba6c1e5160f2b9d Mon Sep 17 00:00:00 2001 From: rupam-04 Date: Wed, 3 Jul 2024 23:11:54 +0530 Subject: [PATCH 4/4] fix: fixed areas where ``err.Error()`` was called when ``err`` is empty --- beacon-chain/rpc/eth/light-client/handlers.go | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/beacon-chain/rpc/eth/light-client/handlers.go b/beacon-chain/rpc/eth/light-client/handlers.go index 17653face06e..087fe191a3cc 100644 --- a/beacon-chain/rpc/eth/light-client/handlers.go +++ b/beacon-chain/rpc/eth/light-client/handlers.go @@ -154,16 +154,24 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R } block, err = s.Blocker.Block(ctx, blockRoot[:]) - if err != nil || block == nil { + if err != nil { httputil.HandleError(w, "Could not get block: "+err.Error(), http.StatusInternalServerError) return } + if block == nil { + httputil.HandleError(w, "Could not get block", http.StatusInternalServerError) + return + } syncAggregate, err := block.Block().Body().SyncAggregate() - if err != nil || syncAggregate == nil { + if err != nil { httputil.HandleError(w, "Could not get sync aggregate: "+err.Error(), http.StatusInternalServerError) return } + if syncAggregate == nil { + httputil.HandleError(w, "Could not get sync aggregate", http.StatusInternalServerError) + return + } if syncAggregate.SyncCommitteeBits.Count() < 1 { // Not enough votes @@ -182,10 +190,14 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R // Get attested state attestedRoot := block.Block().ParentRoot() attestedBlock, err := s.Blocker.Block(ctx, attestedRoot[:]) - if err != nil || attestedBlock == nil { + if err != nil { httputil.HandleError(w, "Could not get attested block: "+err.Error(), http.StatusInternalServerError) return } + if attestedBlock == nil { + httputil.HandleError(w, "Could not get attested block", http.StatusInternalServerError) + return + } attestedSlot := attestedBlock.Block().Slot() attestedState, err := s.Stater.StateBySlot(ctx, attestedSlot) @@ -246,22 +258,26 @@ func (s *Server) GetLightClientFinalityUpdate(w http.ResponseWriter, req *http.R state, err := s.Stater.StateBySlot(ctx, block.Block().Slot()) if err != nil { - httputil.HandleError(w, "could not get state: "+err.Error(), http.StatusInternalServerError) + httputil.HandleError(w, "Could not get state: "+err.Error(), http.StatusInternalServerError) return } // Get attested state attestedRoot := block.Block().ParentRoot() attestedBlock, err := s.Blocker.Block(ctx, attestedRoot[:]) - if err != nil || attestedBlock == nil { - httputil.HandleError(w, "could not get attested block: "+err.Error(), http.StatusInternalServerError) + if err != nil { + httputil.HandleError(w, "Could not get attested block: "+err.Error(), http.StatusInternalServerError) + return + } + if attestedBlock == nil { + httputil.HandleError(w, "Could not get attested block", http.StatusInternalServerError) return } attestedSlot := attestedBlock.Block().Slot() attestedState, err := s.Stater.StateBySlot(ctx, attestedSlot) if err != nil { - httputil.HandleError(w, "could not get attested state: "+err.Error(), http.StatusInternalServerError) + httputil.HandleError(w, "Could not get attested state: "+err.Error(), http.StatusInternalServerError) return }