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

Use cbor-gen encoding for metadata instead of go-ipld-prime #96

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

Use cbor-gen encoding for speed bump working with metadata structs

Implementation

Memory profiles indicate assembling Metadata IPLD structs to encode to bytes consumed significant memory in benchmarks:
profile001

Moreover, fuzz testing has found scenarios where this can be used to panic the decoder: LeastAuthority/go-ipld-prime#2

Switching to using cbor-gen encodings provides a significant performance benefit, and cbor-gen already has significant fuzz testing.

With the change (note almost 150MB less memory used in benchmark):
profile003

Use cbor-gen encoding for speed bump on metadata
@hannahhoward hannahhoward merged commit abebf0f into master Sep 22, 2020
@rvagg
Copy link
Member

rvagg commented Sep 23, 2020

@hannahhoward cbor-gen makes some strictness tradeoffs as part of its speed improvements; most notably it'll ignore trailing bytes after it's parsed the structures out that it expects. Within Filecoin, to deal with this there's an attempt to re-encode everything that comes from the outside (untrusted) to ensure that canonical forms of data structures. This is fine for use-cases, but opens security (or other) holes where canonicity matters and this isn't properly dealt with. Also cbor-gen doesn't do maps properly (according to dag-cbor and the CBOR spec). Maps are avoided in Filecoin alltogether, using tuples everywhere, and there was a proposal to remove that functionality entirely from cbor-gen so as not to provide a potentially confusing option. I've proposed at least fixing it (whyrusleeping/cbor-gen#42) but I don't see much appetite for tinkering with cbor-gen at this late stage (for Filecoin) so that's probably not going anywhere.

I don't know whether either of these things impact graphsync, you're in the best position to make that call. I just wanted to make sure you're aware and are comfortable with those tradeoffs.

@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
@mvdan mvdan deleted the feat/switch-to-cbor-gen branch December 15, 2021 14:16
marten-seemann pushed a commit that referenced this pull request Mar 2, 2023
* Emit events with received cids (#71)

* persist received cids on channel state.

* Send, Receive and Validate Restart requests (#75)

* Send, Receive and Validate Requests

* Initiating and Responding Tests and bug fixes (#76)

* Testing for resuming data transfer work

* Cleanup Push Restarts PR  (#79)

* cleanup of restart PR

* link the peers

* Tests for pull restarts (#84)

* tests for pull restarts

* Merge Tests cleanup work (#92)

* cleanup of restart PR

* cleanup timedout channels (#93)

* backward compatibility of restart (#96)

* backward compatibility of restart

* changes and tests

* more tests

* better error handling for restarts

* feat(message): switch to cbor map encoding (#97)

switch to cbor map encoding for the 1_1 message protocol

* feat(channels): setup datastore migrations (#99)

setup datatransfer channels so they migrate over successfully

Co-authored-by: Hannah Howard <[email protected]>
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

Successfully merging this pull request may close these issues.

2 participants