From a20ab910bcccb7415a6b20d66394795ddaad5889 Mon Sep 17 00:00:00 2001 From: Linus Gasser Date: Fri, 8 May 2020 17:13:51 +0200 Subject: [PATCH] Ignore non-existing instances in GetUpdates (#2263) Currently GetUpdates returns an error if it is queried for an unknown instance. This is not correct, as instances can disappear. It should either ignore them, or return a proof of missing instance. Both is possible, depending on the flag sent to GetUpdates --- byzcoin/api_test.go | 41 ------------------------------- byzcoin/service.go | 24 ++++++++++++++----- byzcoin/service_test.go | 53 +++++++++++++++++++++++++++++++++++++++++ byzcoin/struct.go | 3 +++ 4 files changed, 74 insertions(+), 47 deletions(-) diff --git a/byzcoin/api_test.go b/byzcoin/api_test.go index 0fb5aa446c..958b0fad6e 100644 --- a/byzcoin/api_test.go +++ b/byzcoin/api_test.go @@ -351,47 +351,6 @@ func TestClient_SignerCounterDecoder(t *testing.T) { require.Error(t, c.signerCounterDecoder(buf, &reply)) } -func TestClient_GetUpdates(t *testing.T) { - s := newSer(t, 1, testInterval) - defer s.local.CloseAll() - - req := &GetUpdatesRequest{ - Instances: nil, - Flags: 0, - LatestBlockID: nil, - } - _, err := s.service().GetUpdates(req) - require.Error(t, err) - - req.Instances = []IDVersion{{ConfigInstanceID, 0}} - req.LatestBlockID = s.genesis.SkipChainID() - gur, err := s.service().GetUpdates(req) - require.NoError(t, err) - require.Equal(t, 0, len(gur.Proofs)) - - req.Flags = GUFSendVersion0 - gur, err = s.service().GetUpdates(req) - require.NoError(t, err) - require.Equal(t, 1, len(gur.Proofs)) - require.True(t, gur.Proofs[0].Match(ConfigInstanceID[:])) - - ctx, _ := createConfigTxWithCounter(t, time.Millisecond*100, *s.roster, - 1e5, s, 1) - s.sendTxAndWait(t, ctx, 10) - - req.Flags = 0 - _, err = s.service().GetUpdates(req) - require.Error(t, err) - latest, err := s.service().db().GetLatest(s.genesis) - req.LatestBlockID = latest.Hash - require.NoError(t, err) - - gur, err = s.service().GetUpdates(req) - require.NoError(t, err) - require.Equal(t, 1, len(gur.Proofs)) - require.True(t, gur.Proofs[0].Match(ConfigInstanceID[:])) -} - const testServiceName = "TestByzCoin" type corruptedService struct { diff --git a/byzcoin/service.go b/byzcoin/service.go index 675eb5f2a7..86874f1987 100644 --- a/byzcoin/service.go +++ b/byzcoin/service.go @@ -624,17 +624,29 @@ func (s *Service) GetUpdates(pr *GetUpdatesRequest) (*GetUpdatesReply, error) { sendVersion0 := pr.Flags&GUFSendVersion0 > 0 reply := &GetUpdatesReply{} for _, idv := range pr.Instances { - _, ver, _, _, err := st.GetValues(idv.ID[:]) + proof, err := st.GetProof(idv.ID[:]) if err != nil { - return nil, fmt.Errorf("couldn't read values of instance: %v", err) + return nil, fmt.Errorf("error while looking up proof: %v", err) } - if ver <= idv.Version && - !(sendVersion0 && ver == 0) { + + // Only send missing proofs if flag is set + if !proof.Match(idv.ID[:]) { + if pr.Flags&GUFSendMissingProofs > 0 { + reply.Proofs = append(reply.Proofs, *proof) + } continue } - proof, err := st.GetProof(idv.ID[:]) + + // Check if it's a new version + var inst StateChangeBody + err = protobuf.Decode(proof.Get(idv.ID[:]), &inst) if err != nil { - return nil, fmt.Errorf("error while looking up proof: %v", err) + return nil, fmt.Errorf("invalid instance stored in trie: %v", + err) + } + if inst.Version <= idv.Version && + !(sendVersion0 && inst.Version == 0) { + continue } reply.Proofs = append(reply.Proofs, *proof) } diff --git a/byzcoin/service_test.go b/byzcoin/service_test.go index 12eca0e856..a6baa3277d 100644 --- a/byzcoin/service_test.go +++ b/byzcoin/service_test.go @@ -2536,6 +2536,59 @@ func TestService_Repair(t *testing.T) { require.Equal(t, finalRoot, newRoot) } +func TestService_GetUpdates(t *testing.T) { + s := newSer(t, 1, testInterval) + defer s.local.CloseAll() + + req := &GetUpdatesRequest{ + Instances: nil, + Flags: 0, + LatestBlockID: nil, + } + _, err := s.service().GetUpdates(req) + require.Error(t, err) + + req.Instances = []IDVersion{{ConfigInstanceID, 0}} + req.LatestBlockID = s.genesis.SkipChainID() + gur, err := s.service().GetUpdates(req) + require.NoError(t, err) + require.Equal(t, 0, len(gur.Proofs)) + + req.Flags = GUFSendVersion0 + gur, err = s.service().GetUpdates(req) + require.NoError(t, err) + require.Equal(t, 1, len(gur.Proofs)) + require.True(t, gur.Proofs[0].Match(ConfigInstanceID[:])) + + ctx, _ := createConfigTxWithCounter(t, time.Millisecond*100, *s.roster, + 1e5, s, 1) + s.sendTxAndWait(t, ctx, 10) + + req.Flags = 0 + _, err = s.service().GetUpdates(req) + require.Error(t, err) + latest, err := s.service().db().GetLatest(s.genesis) + req.LatestBlockID = latest.Hash + require.NoError(t, err) + + gur, err = s.service().GetUpdates(req) + require.NoError(t, err) + require.Equal(t, 1, len(gur.Proofs)) + require.True(t, gur.Proofs[0].Match(ConfigInstanceID[:])) + + invID := sha256.Sum256([]byte("invalid instance")) + req.Instances = append(req.Instances, IDVersion{invID, 0}) + gur, err = s.service().GetUpdates(req) + require.NoError(t, err) + require.Equal(t, 1, len(gur.Proofs)) + + req.Flags = GUFSendMissingProofs + gur, err = s.service().GetUpdates(req) + require.NoError(t, err) + require.Equal(t, 2, len(gur.Proofs)) + require.False(t, gur.Proofs[1].Match(invID[:])) +} + func createBadConfigTx(t *testing.T, s *ser, intervalBad, szBad bool) (ClientTransaction, ChainConfig) { switch { case intervalBad: diff --git a/byzcoin/struct.go b/byzcoin/struct.go index d3ebbf61f2..d02bc15142 100644 --- a/byzcoin/struct.go +++ b/byzcoin/struct.go @@ -725,4 +725,7 @@ const ( // GUFSendVersion0 will make GetUpdates to send all instances with // version 0, even those that are note updated. GUFSendVersion0 = GetUpdatesFlags(1 << iota) + // GUFSendMissingProofs will make GetUpdates send proofs for missing + // instances. If not present, missing instances are ignored. + GUFSendMissingProofs )