Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a nice record type #10

Open
Stebalien opened this issue Jan 9, 2018 · 5 comments
Open

Provide a nice record type #10

Stebalien opened this issue Jan 9, 2018 · 5 comments
Labels
status/deferred Conscious decision to pause or backlog

Comments

@Stebalien
Copy link
Member

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
Copy link
Member Author

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.

@anacrolix anacrolix self-assigned this Dec 6, 2018
@anacrolix
Copy link
Contributor

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;
}

@Stebalien
Copy link
Member Author

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.

@anacrolix
Copy link
Contributor

@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.

@Stebalien
Copy link
Member Author

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.

@anacrolix anacrolix added the status/deferred Conscious decision to pause or backlog label Jan 22, 2019
@anacrolix anacrolix removed their assignment Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/deferred Conscious decision to pause or backlog
Projects
None yet
Development

No branches or pull requests

2 participants