go-libp2p-record icon indicating copy to clipboard operation
go-libp2p-record copied to clipboard

Provide a nice record type

Open Stebalien opened this issue 7 years ago • 5 comments

Motivation:

  • Nicer types (e.g., typed author). Working with types generated by the protobuf package isn't exactly fun.
  • Impossible to do stupid things like trust the author field when there's no valid signature.
  • Avoids exposing other unrelated fields rid of any other unrelated fields (e.g., TimeReceived which only exists for local record keeping).
  • Ensure we only store the fields we care about (prevent the DHT from being used as a general purpose KV).

Stebalien avatar Jan 09 '18 22:01 Stebalien

Note: this is now about exposing a type to use elsewhere. I'd like to rename ValidationRecord to simply Record and use it outside of this library instead of passing around the protobuf.

However, this is kind of a low priority.

Stebalien avatar Feb 08 '18 00:02 Stebalien

It looks like the same protobuf message is used on the wire, and in storage. But the extra field timeReceived is only used in storage. Would a change to the binary layout be unworkable due to backwards compatibility?

Ideally I think we would want this:

message Record {
	// The key that references this record
	bytes key = 1;

	// The actual value this record is storing
	bytes value = 2;

	// These field numbers were used previously.
	reserved 3 to 5;
}

import "google/protobuf/timestamp.proto";

message StoredRecord {
	Record record = 1;
	google.protobuf.Timestamp received = 2;
}

But I think this causes a binary incompatibility with existing data stores. An alternate that should be binary compatible would be to fork Record:

message Record {
	// The key that references this record
	bytes key = 1;

	// The actual value this record is storing
	bytes value = 2;

	// These field numbers were used previously.
	reserved 3 to 5;
}

message StoredRecord {
	bytes key = 1;
	bytes value = 2;
	reserved 3, 4;
	string timeReceived = 5;
}

anacrolix avatar Dec 06 '18 05:12 anacrolix

Really, we can go either way. The former is more complex because we'd need to get the JS team to agree and then we'd need to create a datastore migration but I won't object if you want to put in the work.

I'm not sure about the utility of the latter. It's not really removing the hack, just kind of hiding it under the covers. But I could be convinced.

Stebalien avatar Dec 06 '18 18:12 Stebalien

@vasco-santos which of these options would suit the JS implementation? I notice https://github.com/libp2p/js-libp2p-record/blob/master/src/record.proto.js which appears to duplicate the protobuf definition in this repo. Would it be better that all implementations share a mutual proto definition for the record sent over the wire? The same would go for the datastore, if that's intended to be compatible between implementations (it doesn't appear so?).

Even if the first isn't worthwhile, the second option (splitting the record) will still act as some type-enforcement that the 2 records should not be interchangeable.

I'm not of the most appropriate place to discuss this.

anacrolix avatar Dec 12 '18 00:12 anacrolix

I'm not of the most appropriate place to discuss this.

If we're not really touching the spec, here is probably fine. Note: if you need to get someone's attention, IRC is probably the place to go.

Stebalien avatar Dec 12 '18 23:12 Stebalien