Merge pull request #1775 from ipfs/fix/ipns-old-record

fix publish fail on prexisting bad record

Author: jbenet

core/core.go

@@ -227,7 +227,7 @@ func (n *IpfsNode) startOnlineServicesWithHost(ctx context.Context, host p2phost
 	n.Exchange = bitswap.New(ctx, n.Identity, bitswapNetwork, n.Blockstore, alwaysSendToPeer)
 
 	// setup name system
-	n.Namesys = namesys.NewNameSystem(n.Routing)
+	n.Namesys = namesys.NewNameSystem(n.Routing, n.Repo.Datastore())
 
 	// setup ipns republishing
 	err = n.setupIpnsRepublisher()
@@ -456,7 +456,7 @@ func (n *IpfsNode) SetupOfflineRouting() error {
 
 	n.Routing = offroute.NewOfflineRouter(n.Repo.Datastore(), n.PrivateKey)
 
-	n.Namesys = namesys.NewNameSystem(n.Routing)
+	n.Namesys = namesys.NewNameSystem(n.Routing, n.Repo.Datastore())
 
 	return nil
 }

fuse/ipns/common.go

@@ -33,7 +33,7 @@ func InitializeKeyspace(n *core.IpfsNode, key ci.PrivKey) error {
 		return err
 	}
 
-	pub := nsys.NewRoutingPublisher(n.Routing)
+	pub := nsys.NewRoutingPublisher(n.Routing, n.Repo.Datastore())
 	if err := pub.Publish(ctx, key, path.FromKey(nodek)); err != nil {
 		return err
 	}

namesys/namesys.go

@@ -4,6 +4,7 @@ import (
 	"strings"
 	"time"
 
+	ds "github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
 	context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
 	ci "github.com/ipfs/go-ipfs/p2p/crypto"
 	path "github.com/ipfs/go-ipfs/path"
@@ -25,15 +26,15 @@ type mpns struct {
 }
 
 // NewNameSystem will construct the IPFS naming system based on Routing
-func NewNameSystem(r routing.IpfsRouting) NameSystem {
+func NewNameSystem(r routing.IpfsRouting, ds ds.Datastore) NameSystem {
 	return &mpns{
 		resolvers: map[string]resolver{
 			"dns":      newDNSResolver(),
 			"proquint": new(ProquintResolver),
 			"dht":      newRoutingResolver(r),
 		},
 		publishers: map[string]Publisher{
-			"/ipns/": NewRoutingPublisher(r),
+			"/ipns/": NewRoutingPublisher(r, ds),
 		},
 	}
 }

namesys/publisher.go

@@ -18,6 +18,7 @@ import (
 	path "github.com/ipfs/go-ipfs/path"
 	pin "github.com/ipfs/go-ipfs/pin"
 	routing "github.com/ipfs/go-ipfs/routing"
+	dhtpb "github.com/ipfs/go-ipfs/routing/dht/pb"
 	record "github.com/ipfs/go-ipfs/routing/record"
 	ft "github.com/ipfs/go-ipfs/unixfs"
 	u "github.com/ipfs/go-ipfs/util"
@@ -37,11 +38,15 @@ var PublishPutValTimeout = time.Minute
 // routing system.
 type ipnsPublisher struct {
 	routing routing.IpfsRouting
+	ds      ds.Datastore
 }
 
 // NewRoutingPublisher constructs a publisher for the IPFS Routing name system.
-func NewRoutingPublisher(route routing.IpfsRouting) *ipnsPublisher {
-	return &ipnsPublisher{routing: route}
+func NewRoutingPublisher(route routing.IpfsRouting, ds ds.Datastore) *ipnsPublisher {
+	if ds == nil {
+		panic("nil datastore")
+	}
+	return &ipnsPublisher{routing: route, ds: ds}
 }
 
 // Publish implements Publisher. Accepts a keypair and a value,
@@ -62,22 +67,58 @@ func (p *ipnsPublisher) PublishWithEOL(ctx context.Context, k ci.PrivKey, value
 
 	_, ipnskey := IpnsKeysForID(id)
 
-	// get previous records sequence number, and add one to it
-	var seqnum uint64
-	prevrec, err := p.routing.GetValues(ctx, ipnskey, 0)
+	// get previous records sequence number
+	seqnum, err := p.getPreviousSeqNo(ctx, ipnskey)
+	if err != nil {
+		return err
+	}
+
+	// increment it
+	seqnum++
+
+	return PutRecordToRouting(ctx, k, value, seqnum, eol, p.routing, id)
+}
+
+func (p *ipnsPublisher) getPreviousSeqNo(ctx context.Context, ipnskey key.Key) (uint64, error) {
+	prevrec, err := p.ds.Get(ipnskey.DsKey())
+	if err != nil && err != ds.ErrNotFound {
+		// None found, lets start at zero!
+		return 0, err
+	}
+	var val []byte
 	if err == nil {
-		e := new(pb.IpnsEntry)
-		err := proto.Unmarshal(prevrec[0].Val, e)
+		prbytes, ok := prevrec.([]byte)
+		if !ok {
+			return 0, fmt.Errorf("unexpected type returned from datastore: %#v", prevrec)
+		}
+		dhtrec := new(dhtpb.Record)
+		err := proto.Unmarshal(prbytes, dhtrec)
 		if err != nil {
-			return err
+			return 0, err
 		}
 
-		seqnum = e.GetSequence() + 1
-	} else if err != ds.ErrNotFound {
-		return err
+		val = dhtrec.GetValue()
+	} else {
+		// try and check the dht for a record
+		ctx, cancel := context.WithTimeout(ctx, time.Second*30)
+		defer cancel()
+
+		rv, err := p.routing.GetValue(ctx, ipnskey)
+		if err != nil {
+			// no such record found, start at zero!
+			return 0, nil
+		}
+
+		val = rv
 	}
 
-	return PutRecordToRouting(ctx, k, value, seqnum, eol, p.routing, id)
+	e := new(pb.IpnsEntry)
+	err = proto.Unmarshal(val, e)
+	if err != nil {
+		return 0, err
+	}
+
+	return e.GetSequence(), nil
 }
 
 func PutRecordToRouting(ctx context.Context, k ci.PrivKey, value path.Path, seqnum uint64, eol time.Time, r routing.IpfsRouting, id peer.ID) error {

namesys/republisher/repub_test.go

@@ -54,7 +54,7 @@ func TestRepublish(t *testing.T) {
 	// have one node publish a record that is valid for 1 second
 	publisher := nodes[3]
 	p := path.FromString("/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn") // does not need to be valid
-	rp := namesys.NewRoutingPublisher(publisher.Routing)
+	rp := namesys.NewRoutingPublisher(publisher.Routing, publisher.Repo.Datastore())
 	err := rp.PublishWithEOL(ctx, publisher.PrivateKey, p, time.Now().Add(time.Second))
 	if err != nil {
 		t.Fatal(err)

namesys/resolve_test.go

@@ -1,10 +1,14 @@
 package namesys
 
 import (
+	"errors"
 	"testing"
+	"time"
 
+	ds "github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
 	context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
 	key "github.com/ipfs/go-ipfs/blocks/key"
+	peer "github.com/ipfs/go-ipfs/p2p/peer"
 	path "github.com/ipfs/go-ipfs/path"
 	mockrouting "github.com/ipfs/go-ipfs/routing/mock"
 	u "github.com/ipfs/go-ipfs/util"
@@ -13,9 +17,10 @@ import (
 
 func TestRoutingResolve(t *testing.T) {
 	d := mockrouting.NewServer().Client(testutil.RandIdentityOrFatal(t))
+	dstore := ds.NewMapDatastore()
 
 	resolver := NewRoutingResolver(d)
-	publisher := NewRoutingPublisher(d)
+	publisher := NewRoutingPublisher(d, dstore)
 
 	privk, pubk, err := testutil.RandTestKeyPair(512)
 	if err != nil {
@@ -43,3 +48,90 @@ func TestRoutingResolve(t *testing.T) {
 		t.Fatal("Got back incorrect value.")
 	}
 }
+
+func TestPrexistingExpiredRecord(t *testing.T) {
+	dstore := ds.NewMapDatastore()
+	d := mockrouting.NewServer().ClientWithDatastore(context.Background(), testutil.RandIdentityOrFatal(t), dstore)
+
+	resolver := NewRoutingResolver(d)
+	publisher := NewRoutingPublisher(d, dstore)
+
+	privk, pubk, err := testutil.RandTestKeyPair(512)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	id, err := peer.IDFromPublicKey(pubk)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Make an expired record and put it in the datastore
+	h := path.FromString("/ipfs/QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN")
+	eol := time.Now().Add(time.Hour * -1)
+	err = PutRecordToRouting(context.Background(), privk, h, 0, eol, d, id)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Now, with an old record in the system already, try and publish a new one
+	err = publisher.Publish(context.Background(), privk, h)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = verifyCanResolve(resolver, id.Pretty(), h)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestPrexistingRecord(t *testing.T) {
+	dstore := ds.NewMapDatastore()
+	d := mockrouting.NewServer().ClientWithDatastore(context.Background(), testutil.RandIdentityOrFatal(t), dstore)
+
+	resolver := NewRoutingResolver(d)
+	publisher := NewRoutingPublisher(d, dstore)
+
+	privk, pubk, err := testutil.RandTestKeyPair(512)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	id, err := peer.IDFromPublicKey(pubk)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Make a good record and put it in the datastore
+	h := path.FromString("/ipfs/QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN")
+	eol := time.Now().Add(time.Hour)
+	err = PutRecordToRouting(context.Background(), privk, h, 0, eol, d, id)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Now, with an old record in the system already, try and publish a new one
+	err = publisher.Publish(context.Background(), privk, h)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = verifyCanResolve(resolver, id.Pretty(), h)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func verifyCanResolve(r Resolver, name string, exp path.Path) error {
+	res, err := r.Resolve(context.Background(), name)
+	if err != nil {
+		return err
+	}
+
+	if res != exp {
+		return errors.New("got back wrong record!")
+	}
+
+	return nil
+}