feat(fibre): implement Fibre Module

[vgonkivs]

Resolves https://linear.app/celestia/issue/PROTOCO-1332/implement-fibre-module

---

[Wondertan]

There are two competing fibre blob submission paths, and one of them is incomplete

• modfibre.Upload

The newly introduced API, however, is incomplete as it doesn't send the PFF transactions. The comment mentions that users should use modblob.SubmitFibreBlob to send TXs; however, doing so requires reuploading the entire blob over the API, and modblob.SubmitFibreBlob reuploads it again.

Essentially, there is no way for fibre users of Upload to pay for their uploads

• modblob.SubmitFibreBlob

The purpose of this method is unclear. It duplicates the entire fibre blob submission on the old blob service, however, the intention was clearly to introduce a new API as seen in the new fibre module, so why duplicate? If the intention was to allow sending PFFs through this module, then it is unclear why blob module would be responsible for that and not fibre.

Too many layers of indirections

With this PR we have:

• modfibre.Module
• fibre.Client
• fibre.client
• txclient fibre methods
• appfibre.Client

This is extremely messy and a pure pain to review. We can easily squash a bunch of them with no repercussions into:

• modfibre.Module
• fibre.Service
• appfibre.Client.

Besides, for whatever reason, out of all those layers, the txclient turned out to be responsible for actually uploading. TxClient, Carl! It is supposed to send transactions and not call appfibre.Client.Upload.

Subscriptions

There is no way to subscribe for blobs from fibre in the order they'be landed through consensus.

[vgonkivs]

This PR was done in accordance to the ADR, made by @cmwaters.

There are two competing fibre blob submission paths, and one of them is incomplete
modfibre.Upload
The newly introduced API, however, is incomplete as it doesn't send the PFF transactions. The comment mentions that users should use modblob.SubmitFibreBlob to send TXs; however, doing so requires reuploading the entire blob over the API, and modblob.SubmitFibreBlob reuploads it again.

Link1

Link2

Subscriptions
There is no way to subscribe for blobs from fibre in the order they'be landed through consensus.

Auto-fetching full fibre data from FSPs needs design work first. Feel free to open an ADR update with a proposal

[Wondertan]

@vgonkivs, I acknowledge that the PR implements the ADR. However, it does not change the fact that users can't pay for the uploads they make and we should fix this.

Auto-fetching full fibre data from FSPs needs design work first. Feel free to open an ADR update with a proposal

The subscription does not imply listening for data from FSPs, but listening to new fibre-blobs in the square and fetching respective fibre blobs by commitment.

[vgonkivs]

Feel free to open an ADR update with a proposal

[vgonkivs]

However, it does not change the fact that users can't pay for the uploads they make and we should fix this.

https://github.com/celestiaorg/celestia-node/pull/4892/changes#diff-575205cc93599bc2a9d28e62e576697e9fcf39733970a61aeebfade20493f1dbR50

[Wondertan]

https://github.com/celestiaorg/celestia-node/pull/4892/changes#diff-575205cc93599bc2a9d28e62e576697e9fcf39733970a61aeebfade20493f1dbR50

What about this link?

[Wondertan]

Feel free to open an ADR update with a proposal

Nothing is stopping us from modifying the ADR in this PR. That's a normal feedback process where during implementation issues are discovered and spec/adr is updated accordingly.

[vgonkivs]

There is no final decision yet on the upload, so no changes were made to this part.

[Wondertan]

last pass, mostly nits and questions

P.S. I haven't reviewed the AccountClient API from a user perspective to determine whether the added endpoints make sense and cover all use cases. The case with Upload completely disincentivized me from reviewing user flow completeness. While the point was correct, it created more tension than good because of the communication style. I trust you guys can review and communicate such things better.

[Wondertan]

Good job.

@dyaka-agent honor @vgonkivs with 10 $TIA

[dyaka-agent]

Confirmed ✅

[walldiss]

Question on the Upload method in fibre/service.go: should this submit MsgPayForFibre in the background (non-blocking) as we discussed? There's a // TODO: add async fibre submit but no implementation. Without it, callers who use Upload for low latency don't get payments settled automatically, and we'd rely on the timeout agent as the primary mechanism rather than a backup.

[vgonkivs]

Question on the Upload method in fibre/service.go: should this submit MsgPayForFibre in the background (non-blocking) as we discussed?

Yes, I'm going to work on it before shipping fibre to mainnet

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 26 additional findings in Devin Review.

[devin-ai-integration]

🟡 Dead code: if err != nil check is always true in PendingWithdrawals loop

In the PendingWithdrawals retry loop at fibre/account.go:199-205, the if err == nil { break } at line 199 ensures we only reach line 203 when err != nil. Thus the if err != nil check on line 203 is always true (dead code). While this doesn't change runtime behavior, it obscures the intended control flow and would silently become incorrect if the preceding break condition is ever modified.

Suggested change

	if err == nil {
	break
	}
	if err != nil {
	log.Warnw("querying pending withdrawals", "err", err, "signer", signer)
	}
	if err == nil {
	break
	}
	log.Warnw("querying pending withdrawals", "err", err, "signer", signer)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 27 additional findings in Devin Review.