spec: Shwap #3205

Wondertan · 2024-02-23T16:03:47Z

Introduces Shwap page that points to the CIP. This PR integrates it nicely with mdbook engine like it is a local document in the node. It integrates multiple mdbook preprocessors into rendering pipeline and GH CI.

Rendered on GH pages

github-actions · 2024-02-23T16:04:04Z

PR Preview Action v1.4.7
🚀 Deployed preview to https://celestiaorg.github.io/celestia-node/pr-preview/pr-3205/
on branch `gh-pages` at 2024-03-26 16:57 UTC

codecov-commenter · 2024-02-23T16:10:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.59%. Comparing base (1809680) to head (5a818be).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3205      +/-   ##
==========================================
- Coverage   51.92%   51.59%   -0.34%     
==========================================
  Files         178      178              
  Lines       11316    11316              
==========================================
- Hits         5876     5838      -38     
- Misses       4942     4971      +29     
- Partials      498      507       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oblique · 2024-02-28T11:25:58Z

specs/src/shwap/spec.md

+Identifiers are embeddable to narrow down to the needed content. (TODO: Describe better)
+
+Identifiers MUST have a fixed size for their fields. Subsequently, protobuf can't be used for CID serialization due to
+varint usage. Instead, identifiers use simple binary big endian serialization.


In your current Shwap PR little endian is used. We also implemented it with little endian. Which one should we use?

We will use the big endian. I did a slight research on that and found out that its is used in IP and QUIC with rationale that its just faster on most architectures, so decided to follow that here.

zvolin · 2024-02-28T11:34:35Z

what would you say for enforcing the package fields in proto definitions in the spec?

Wondertan · 2024-02-28T11:43:42Z

@zvolin, I find it unnecessary, but might missing some context from other languages.

zvolin · 2024-02-28T12:17:33Z

so it usually affects the namespace in which the generated types will be. In our case we are generating and exposing all protos in a single crate (package) and all the messages without a package will be in a root namespace (even if they are generated separately). It sounds like something that will not hurt but can be useful.
In celestia-app protos are organized using pattern celestia.module.version and that ends up in types like celestia::blob::v1::MsgPayForBlobs in rust.
In node they usually look similar to the go module they are in, so we have header::pb::ExtendedHeader or share::p2p::shrex::nd::GetSharesByNamespaceRequest.
It would look nice on our end if shwap would have package share.p2p.shwap or something like so to be consistent.

oblique · 2024-02-28T12:32:25Z

specs/src/shwap/spec.md

+### Bitswap CID integration
+
+The naive question would be: "Why not to make content verification after Bitswap provided it back over its API?"
+Intuitively, this would simplify a lot and wouldn't require "hacking" CID. However, this has an important downside - 
+the Bitswap in such case would consider the content as fetched and valid, sending DONT_WANT message to its peers, while
+the message might be invalid according to the verification rules.


Because the verification is done in the Bitswap level, the only way knowing that data sampling failed and block should be rejected, is to have a timeout. So, how many seconds the timeout should be? Should we define it here?

Interesting thought. I think this timeout is out of Shwap's scope. The actual sampling logic - how, how many samples are selected, and what the timeout for sampling deserves as separate spec/cip that depends on Shwap protocol.

Shwap is a transport for specialized DA messaging. The sampling logic/details are out of its scope

Wondertan · 2024-02-28T13:35:40Z

@zvolin, my hesitation with package share.p2p.shwap is that it enforces a certain package structure for the client implementations. I aim for the spec to be as language-agnostic as possible and broadly free from any non-protocol constraints.

Making all the proto follow the same package forces all the types to be generated in a single place, like what the core and lumina are doing. Node on the other hand is more modular and we keep proto-types together with the protocols, where each protocol is its own independent module/package. This is an example of an architecture decision that implementers are free to make and enforcing package structure through package directive breaks that.

It's nice to be consistent and I see why consistency is good to follow in general, but not in this case.

walldiss · 2024-03-01T13:00:52Z

specs/src/shwap/spec.md

+The current Data Availability Sampling (DAS) network protocol is inefficient. A _single_ sample operation takes log2(k) network
+round-trips(where k is the square size). This is not practical and does not scale for the theoretically unlimited data
+square that the Celestia network enables. The main motive here is a protocol with O(1) round-trip for _multiple_ samples, preserving
+the assumption of having 1/n honest peers connected.


Does "1/n" means that there needs to be at least one peer out of N, or something else?
Also, clarification is needed on whether peers must be both honest and possess the requested data shares. For example, an honest light node might lack the relevant data shares, impacting sampling efficiency. Clarifying these points would strengthen the protocol's specifications.

Does "1/n" means that there needs to be at least one peer out of N, or something else?

1 honest peer.

Also, clarification is needed on whether peers must be both honest and possess the requested data shares

1/n is a pretty common assumption in p2p networks. In this context honesty assumes possession. Nevertheless, clarifying it makes. Thanks for pointing this out.

walldiss · 2024-03-01T13:07:19Z

specs/src/shwap/spec.md

+### Multihashes and CID
+
+Shwap takes inspiration from content addressability, but breaks-free from hash-based only model to optimize message sizes
+and data request patterns. In some way, it hacks into multihash abstraction to make it contain data that isn't in fact a


Suggested change

and data request patterns. In some way, it hacks into multihash abstraction to make it contain data that isn't in fact a

and data request patterns. In some way, it hacks into multihash abstraction to make it contain arbitrary message fields that isn't in fact a

We might want to be more specific here

Not sure how this change anything to message I tried to convery. Arbitrary message fields are also quite general and can mean anything.

Maybe something like "identification data" may improve it

walldiss · 2024-03-01T13:09:51Z

specs/src/shwap/spec.md

+
+Identifiers are embeddable to narrow down to the needed content. (TODO: Describe better)
+
+Identifiers MUST have a fixed size for their fields. Subsequently, protobuf can't be used for CID serialization due to


Perhaps it is worth to explain that the reason why Identifiers MUST have a fixed size is bitswap internal architecture.

That's a good point. I really would like not to have bitswap as a rationale for this, as this breaks Shwap protocol independence. Ideally, we would find a way to avoid this limitation until the CIP is finalized or find another rationale for why this is necessary.

walldiss · 2024-03-01T13:13:23Z

specs/src/shwap/spec.md

+| Name     | Multihash | Codec  |
+|----------|-----------|--------|
+| RowID    | 0x7811    | 0x7810 |
+| SampleID | 0x7801    | 0x7800 |
+| DataID   | 0x7821    | 0x7820 |


Just for nice aligment, why not those not ordered by desc multihash/codec?

I agree. I wanted to keep the order of IDs as defined in the spec and I didn't want to break what was already implemented, but as we break other things, it may not hurt to break this one.

walldiss · 2024-03-01T13:43:03Z

specs/src/shwap/spec.md

+
+Shwap takes inspiration from content addressability, but breaks-free from hash-based only model to optimize message sizes
+and data request patterns. In some way, it hacks into multihash abstraction to make it contain data that isn't in fact a
+hash. Furthermore, the protocol does not include hash digests in the multihashes. The authentication of the messages


Suggested change

hash. Furthermore, the protocol does not include hash digests in the multihashes. The authentication of the messages

hash. Furthermore, the protocol does not include hash digests in the multihashes. The verification of the messages

My understanding is that authentication is verifying who provided data, rather than content in response. What do you think to replace authentication -> verification

Both terms work in this case. I've seen data/message authentication used to describe data that is not being tampered with and is coming from the right source. In this case, we authenticate messages against DAH, which assures us the data is correct and is coming from/produced by consensus/validators

… in practice

Wondertan · 2024-03-04T21:39:05Z

Noting that spec fully migrated to celestiaorg/CIPs#77, so all the comments and discussions should move there as well. Check the description for more on new changes here

Wondertan · 2024-03-04T21:47:28Z

specs/.gitignore

@@ -1 +1,2 @@
 book
+theme


@rootulp, the theme dir can be similarly .gitignored in the app's repo

thanks! Created celestiaorg/celestia-app#3151

Wondertan added 2 commits February 23, 2024 16:57

spec: Shwap

bd8d7a2

add abstract and motivatin

5a818be

Wondertan added the specs label Feb 23, 2024

Wondertan self-assigned this Feb 23, 2024

Wondertan requested review from renaynay, vgonkivs, distractedm1nd, walldiss and ramin as code owners February 23, 2024 16:03

Wondertan added the kind:docs For solely documentation PRs label Feb 23, 2024

Wondertan requested review from adlerjohn and liamsi February 26, 2024 13:18

oblique reviewed Feb 28, 2024

View reviewed changes

walldiss reviewed Mar 1, 2024

View reviewed changes

Wondertan added 9 commits March 1, 2024 16:31

streamline spec and add more sections

7d02fb7

format and lint

7ea064b

update bitswap section

0644ea7

update bitswap section

dc3725e

grammar fixes

03ae4b8

migrate spec to CIP

1a78146

migrate spec to CIP

15d620c

prettify spec rendering

da9a711

gh pages build preprocessers for mdbok

4d9ce37

Wondertan added 5 commits March 4, 2024 21:51

try different action with caching

a62ca92

use separate step for each binary

417a6e7

fix attempt

cfebc8a

fix mdbook-pagetoc

bae3b65

update step names and trigger rebuild of preprocessors to see caching…

59587c3

… in practice

new line to gitignore

6a523fc

Wondertan commented Mar 4, 2024

View reviewed changes

rootulp mentioned this pull request Mar 4, 2024

Ignore mdbook theme/ celestiaorg/celestia-app#3151

Closed

Wondertan mentioned this pull request Mar 5, 2024

CIP-19: Shwap celestiaorg/CIPs#77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spec: Shwap #3205

spec: Shwap #3205

Wondertan commented Feb 23, 2024 •

edited

github-actions bot commented Feb 23, 2024 •

edited

codecov-commenter commented Feb 23, 2024

oblique Feb 28, 2024 •

edited

Wondertan Feb 28, 2024 •

edited

zvolin commented Feb 28, 2024

Wondertan commented Feb 28, 2024

zvolin commented Feb 28, 2024

oblique Feb 28, 2024

Wondertan Feb 28, 2024

Wondertan Feb 28, 2024

Wondertan commented Feb 28, 2024 •

edited

walldiss Mar 1, 2024

Wondertan Mar 5, 2024

walldiss Mar 1, 2024

Wondertan Mar 5, 2024

walldiss Mar 1, 2024

Wondertan Mar 5, 2024

walldiss Mar 1, 2024

Wondertan Mar 5, 2024

walldiss Mar 1, 2024

Wondertan Mar 5, 2024

Wondertan commented Mar 4, 2024 •

edited

Wondertan Mar 4, 2024

rootulp Mar 4, 2024

	and data request patterns. In some way, it hacks into multihash abstraction to make it contain data that isn't in fact a
	and data request patterns. In some way, it hacks into multihash abstraction to make it contain arbitrary message fields that isn't in fact a


		Identifiers are embeddable to narrow down to the needed content. (TODO: Describe better)

		Identifiers MUST have a fixed size for their fields. Subsequently, protobuf can't be used for CID serialization due to

	hash. Furthermore, the protocol does not include hash digests in the multihashes. The authentication of the messages
	hash. Furthermore, the protocol does not include hash digests in the multihashes. The verification of the messages

spec: Shwap #3205

Are you sure you want to change the base?

spec: Shwap #3205

Conversation

Wondertan commented Feb 23, 2024 • edited

github-actions bot commented Feb 23, 2024 • edited

codecov-commenter commented Feb 23, 2024

Codecov Report

oblique Feb 28, 2024 • edited

Choose a reason for hiding this comment

Wondertan Feb 28, 2024 • edited

Choose a reason for hiding this comment

zvolin commented Feb 28, 2024

Wondertan commented Feb 28, 2024

zvolin commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan commented Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan commented Mar 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan commented Feb 23, 2024 •

edited

github-actions bot commented Feb 23, 2024 •

edited

oblique Feb 28, 2024 •

edited

Wondertan Feb 28, 2024 •

edited

Wondertan commented Feb 28, 2024 •

edited

Wondertan commented Mar 4, 2024 •

edited