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

Don't allow setting arbitrary PrevID for records #525

Open
merlinran opened this issue Jun 4, 2021 · 3 comments
Open

Don't allow setting arbitrary PrevID for records #525

merlinran opened this issue Jun 4, 2021 · 3 comments

Comments

@merlinran
Copy link
Contributor

Currently adding an arbitrary record is allowed (https://pkg.go.dev/github.com/textileio/go-threads@v1.0.2#readme-adding-a-thread-record) so I made the mistake of creating records using other logs' records as Prev in the testground test, and alas, the test passes sometimes, but causes all these problems of lock contention etc. So after clarifying w/ @sanderpick and @carsonfarmer it's pretty clear that when creating, a new record should only set the current log head as its Prev, because a log is just a chain of records created by the log's owner.

The simplest would be to remove the Prev field in the cbor.CreateRecordConfig, or, to avoid compatibility issues, override it and print a warning if it doesn't equal to the current head. In later stage we could probably have a better API which can get rid of AddRecord altogether.

@merlinran
Copy link
Contributor Author

Well we can't remove the Prev field in the cbor.CreateRecordConfig because signing requires it. Guess we have to postpone to API change to have a log abstraction, for example, so we can call something like log.AppendRecord in an intuitive way.

@sanderpick
Copy link
Member

Ah, makes sense. I was also wondering something related that @carsonfarmer may know about: Doesn't the threaddb js lib need to "batch" add records that are already chained? I believe that was the origin of the AddRecord API.

Instead of removing the API, we could probably fix the putRecords logic to enforce that either:

  • The first entry's Prev in the chain is HEAD
  • HEAD is somewhere in the chain

That should be enough to prevent creating orphaned chains.

@carsonfarmer
Copy link
Member

Not at the moment @sanderpick, because the JS libs no longer operate at this lower level API (they did when we had a full JS node). You are indeed correct though, that AddRecord was required in JS-land so that remote peers (js-threads) could create Records and then push them to the "remote" (go-threads). We don't do this at the moment, but there is a future where a JSON-RPC wrapped AddRecord API could be used from JS-land again...

I guess TL;DR not a constraint for JS right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants