celestiaorg / celestia-app

Celestia consensus node
https://celestiaorg.github.io/celestia-app/
Apache License 2.0
342 stars 281 forks source link

Merge PFB and Tx Namespaces #3315

Open cmwaters opened 5 months ago

cmwaters commented 5 months ago

Summary

Parent Issue: https://github.com/celestiaorg/celestia-app/issues/2977

This is a proposal to merge the two celestia state machine specific namespaces into a single namespace. This is to be done by modifying the square construction mechanism to populate the TxNamespace with all transaction types including those with PFBs.

Problem Definition

This is meant to address the current emergent problems from when we originally separated into two namespaces:

Proposal

Create a new square construction protocol (v2 of the go-square repo). That places all transactions in TxNamespace and no longer uses the PFBNamespace.

musalbas commented 5 months ago

If I recall correctly, the reason why we separated them into different namespaces in the first place was because we needed to domain-separate the PFB transactions so it's easier to parse by sovereign ZK rollups, as requested by Sovereign SDK. How would this proposal effect that? cc @preston-evans98

nashqueue commented 5 months ago

Note that this might break the FBSS construction, or at least make it more complicated.

preston-evans98 commented 5 months ago

Thanks for flagging @musalbas!

Having all of the PFBs in a separate namespace is nice for parsing (because we can avoid parsing irrelevant transactions), but is not strictly required. IIRC, the core issue with the previous implementation was that standard transactions and PFBs had separate encodings but were not domain separated. That meant that it was impossible to guarantee that a parser was actually recovering the transactions with the appropriate type.

That being said, this change will require significant breaking changes to the sov SDK adapter and will significantly reduce efficiency unless #3316 is also merged - in which case the impact is completely removed.

adlerjohn commented 5 months ago

My understanding is #3316 was planned to go in at the same time (i.e. in the same release) as #3315. You can quote me on this if that ends up not being the case so we can make it the case.

adlerjohn commented 5 months ago

@nashqueue why would this break FBSS?

evan-forbes commented 3 weeks ago

@cmwaters this is not in v3, correct?

rootulp commented 3 weeks ago

Correct. Ref: https://github.com/celestiaorg/go-square/blob/6be96324999598f641bdb20ef0fb7043cece4d95/builder.go#L181-L183