celestiaorg/celestia-app

Celestia consensus node

celestia-app's Issues

Register GRPC Server for lazyledgerapp module

When the app was updated to the latest version of lazyledger-core and the sdk, one of the changes to the sdk was to add the RegisterGRPCServer method to the server.Application interface. This method has yet to be implemented in the app, and results in the app not being accessible via grpc.

state fraud proofs (celestia): verification logic

Verifying a fraud proof requires knowledge of the state logic. We need to investigate which parts will have to live:

1. in the abci app itself
2. in the Cosmos-SDK
3. lazyledger-core

Lazyledger-core most likely should provide a simple library (reactor?) to gossip fraud proofs. Maybe the evidence pool can be re-used for that. This should be as light-weight as possible, as we would want light-clients to also participate in the gossiping.
Fraud proofs will be gossiped by Celestia-nodes: https://github.com/celestiaorg/celestia-node
Additionally, celestia-core needs to collect the intermediate state in Block.Data (celestiaorg/celestia-core#84).

The Cosmos-SDK probably needs modifications to produce these intermediate state roots (celestiaorg/cosmos-sdk#8 and celestiaorg/cosmos-sdk#9).

Fraud proofs generation and verification could happen in a separate module/in the lazyledger-app itself.

The overall design should be part of rollkit/rollkit#23 too. ADRs that link to each other in the particular repositories will inform this design document.

Ideally, the design should be independent of which tree is actually used (iavl/smt/other) (ref: celestiaorg/cosmos-sdk#8)

Add more tests for generating transactions via the CLI

If we would have had more tests that use the cli to generate transactions, then we would have noticed #132 sooner. The way we generate MsgWirePayForMessages is atypical for an cosmos-sdk chain in that we are actually generating and signing multiple iterations of the same transaction. We need to have better testing for this.

This should be completed either when completing #132 or directly after, as completing that will require a refactor of the current way we generate transactions using the cli.

Investigate Rosetta API

Brought up by @nickw321 (thanks): Investigate what it would take to make this abci-app rosetta compatible:
https://blog.cosmos.network/rosetta-api-for-cosmos-sdk-1c67b92da0b3

Might also be relevant to optimint.

Use the latest version of tendermint and the sdk

After updating to the latest version of the cosmos-sdk #27 we should make the required changes to update here as well. A lot has changed since the last version we used, and we will have to make some minor adjustments. It should also be noted that starport does not yet have support for the latest version of the cosmos-sdk, so if we have too much trouble, then we might just want to use the version they use (v0.42.6) instead of the latest version.

blocked by:

• celestiaorg/celestia-core#492
• celestiaorg/cosmos-sdk#27

App is not buildable on OSX Big Sur 11.6

Summary of Bug

keyring lib is throwing errors during make install process

Version

git rev-parse --verify HEAD
d061ef0f782e062a52db16da26bfe974a544a2d9

Steps to Reproduce

1. git clone repo
2. make install
3. Witness
   # github.com/keybase/go-keychain cgo-gcc-prolog:203:11: warning: 'SecTrustedApplicationCreateFromPath' is deprecated: first deprecated in macOS 10.15 - No longer supported [-Wdeprecated-declarations] /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Security.framework/Headers/SecTrustedApplication.h:59:10: note: 'SecTrustedApplicationCreateFromPath' has been explicitly marked deprecated here

Note: If updating the lib then
../../../../pkg/mod/github.com/cosmos/[email protected]/keychain.go:136:8: kcItem.SetAccess undefined (type keychain.Item has no field or method SetAccess)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

slim down abci app

Before launch, we should further strip down the state machine to a bare minimum that supports staking, fees, and transfers fees only. That said, governance and upgrades might also be part of this.

Action items:

• narrow down which modules are the bare minimum and document why they are required in the specs
• remove/disable all other module and delete any unused code

Define genesis block

┆Issue is synchronized with this Asana task by Unito

Implement Integration Tests for WirePayForMessage

The latest cli addition still needs integration tests. This should include

• booting up a single node devnet and ipfs daemon
• posting the data via the cli
• checking that the appropriate fees were burned (which could be 0 depending on which stage we're at)
• check for the data posted via ipfs

Add FOSS license

Preferably Apache-2,0.

Stop messages from using reserved namespaces

Namespaces up to and including NAMESPACE_ID_MAX_RESERVED = 255 are reserved, not messages: https://github.com/lazyledger/lazyledger-specs/blob/de5f4f74f56922e9fa735ef79d9e6e6492a2bad1/specs/consensus.md#constants.
There should also be a check somewhere that a reserved namespace ID that's up to 255 but not specifically reserved is invalid.

Originally posted by @adlerjohn in celestiaorg/celestia-core#261 (comment)

There should be a check in MsgWirePayForMessage's ValidateBasic message to make sure that a message is not using reserved namespaces.

Decide what to do about missing logging flags for the start sub-command

Logging has been refactored in the cosmos-sdk. Starport apps have yet to be updated accordingly, so these flags have to be manually added to the start sub-command in order to pass the logging configurations to lazyledger-core.

I see two very easy solutions to this, we could

1. add the flags to the start cmd in the lazyledger fork of the cosmos-sdk
2. add the flags outside of the sdk in the lazyledger-app
3. Something else that I'm not seeing (@marbar3778 @liamsi)

Ideally, we would add them to the cosmos-sdk, right next to other start sub-command flags. However, any changes we make to the sdk will have to be undone each time we upstream anything.

Readme: starport serve ref

I assume you do not need startport serve to run the app? https://github.com/lazyledger/lazyledger-app/blob/master/readme.md#get-started

Figure out ideal CLI UI

The latest addition to the cli follows the format of the rest of the starport generated cli, except it use relies on hex encoding for accepting the two raw byte arguments. While most users are not going to be using the cli, we still need to figure an ideal, or at least consistent, UI/UX.

Fees: actually enforce deducting those

As far as I remember, currently no fees are deducted for PayForMessage. Is that right? Or do we actually make use of the default fee-machanism of the SDK?

Add CI via github actions

Ideally via github actions:

at least:

• run (go) tests

ideally:

• test coverage reporting
• integration / e2e tests

specs: Define transaction hash

Block header hashes are defined, but not transaction IDs. Need to define specifically how to compute a transaction ID.

Document IBC denomination filter

reproducible builds

Potentially this can be done by following what gaia does (I did some testing with Alessio back in the days but I don't remember the details and the process likely improved and changed over time):

• https://github.com/cosmos/gaia/blob/main/docs/resources/reproducible-builds.md
• https://github.com/tendermint/tendermint/blob/2d57043aa46e38f60c153f6237bf541abe104cf3/Makefile#L287

docs: Add information to the node operator's guide on amount to stake

Summary

We should give a recommendation in the guide on which minimum amount of tokens to add in order to not bump into issues for future node operators

Problem Definition

If we execute something like this celestia-appd gentx <user_n> 100000stake --chain-id <abc> --keyring-backend <xyz>

Then we get the error
Error: error during handshake: error on replay: validator set is nil in genesis and still empty after InitChain This is not directly/explicitly pointing to the issue of insufficient amount of tokens for gentx.

Proposal

In order to avoid future repeated questions from the community, we should include an info field in the guideline that recommends a minimum amount to include in order to not bump into this issue. Something like

celestia-appd gentx <user_n> 1000000stake --chain-id <abc> --keyring-backend <xyz> (1mil instead of 100k)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Repo and command-line tool name

Currently, the binary that runs lazyledger is called lazyledger-appd. We should probably rename this to lazyledger or lazyledgerd?

Also, the repo name could just be lazyledger or lazyledger-rsm (replicated state machine) or some other name we should settle on.

There was also the suggestion to rename the tendermint binaries in lazyledger-core. IMO, their name can just stay the same. The consensus is in fact tendermint consensus and the user-facing binaries are in here.

Also, another name to consider is the one chosen for modules (currently there is only one): https://github.com/lazyledger/lazyledger-app/tree/master/x/lazyledgerapp

cc @musalbas @adlerjohn

Remove the `Nonce` field in `MsgWirePayForMessage`

Currently, the nonce in MsgWirePayForMessage is not being used. Instead, the sdk handles the AccountNumber SequenceNumber for the app. The app should be modified so that it follows the spec and handles the nonce inside of MsgWirePayForMessage, instead of relying on the sdk to handle the AccountNumber SequenceNumber.

Create API to check for message inclusion

Summary

Create an API to check for messages inclusion.

Details

The first step has already been done by writing code to find subtree root paths. The next step should be to create an API that allows us to easily check for message inclusion given the original data square, the position of the message in question, the size of that message, along with the commitment provided in the SignedPayForMessage.

Action Items

• define API for checking inclusion
• compare commitments and subtree roots the SignedPayForData and the actual data in the square given the location and message size.
• write integration tests for subtree root code

References

depends on #49

Potentially increase share size

We currently set the share size (rather arbitrarily) to 256 bytes. IMO this should be increased to maybe 1KB or more. Independent of what I think there must be a rationale document for the parameter choice explaining pros/cons and why we settled on whatever value.

Move MsgWirePayForMessage transaction creation code outside of this repo

Per discussion in #155, we have decided that the code to create MsgWirePayForMessages programmatically will need to exist in a different repo. This is because if the code is imported from this repo, it adds a dependency on celestia-core, which causes dependency hell for any projects that also import tendermint.

Decide how to add functionality to baseapp

We are going to have to add functionality to baseapp #5, specifically implement the new ABCI methods from lazyledger-core. We could simply change baseapp in the cosmos-sdk, but that is highly undesirable. Ideally, our solution should keep as many changes in the lazyledger-app as possible.

I'm proposing that we simply wrap any usage of the baseapp with a type that by default forwards all ABCI methods to the baseapp's methods, except when we need to "overwrite" one, we can without making any change to the sdk.

type wrappedBaseApp struct {
    bApp *baseapp.BaseApp
}

// forward to baseapp's methods
func (w *wrappedBaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {
    return w.bApp.Info(req)
}

func (w *wrappedBaseApp) PreprocessTxs(txs abci.RequestPreprocessTxs) abci.ResponsePreprocessTxs {
	// todo: insert custom code here
	return w.bApp.PreprocessTxs(txs)
}

type App struct {
    *wrappedBaseApp
   ...
}

I have an implementation here, but would like to make sure I'm not doing anything overly cumbersome before I commit.

Adjust share size to match the spec and add namespaces to each Message

Namespaces are now kept in the actual message to ensure that they are recoverable, see the spec PR. This should also be accounted for in the app, specifically SHARE_SIZE - NAMESPACE_ID_BYTES - SHARE_RESERVED_BYTES

Point to ll-core on master

As we do not tag ll-core yet, we should rather point to master imo:
https://github.com/lazyledger/lazyledger-app/blob/1e3ebd5361536f0a171e24b1f378ae649b58c691/go.mod#L26

EPIC: staking: update according to spec

The specification makes some changes to the staking logic used internally in the cosmos-sdk:

• https://github.com/lazyledger/lazyledger-specs/blob/master/specs/consensus.md
• https://github.com/cosmos/cosmos-sdk/tree/master/x/staking

Some of these changes are absolutely required for fraud proofs others are simplifications/optimizations.

Concrete action items for this issue (should be split into multiple issues/prs):

• fully understand and document the diff between the spec and the cosmos-sdk
• triage which parts of above diff are essential to launch LL and which are optimizations
• create issues that describe the changes necessary (and where they have to be implemented: here (preferrably) or in the SDK if necessary)
• implement above changes.

While the implementation can happen as late as the incentivized testnet, all other todos should be tackled earlier as they will determine how much work this actually is.

Implement the simulation package for `x/blob` module

Modules in the sdk all have a package called simulation that interfaces with the simapp framework for fuzzing. Implement the simulation package for lazyledgerapp so that proper simulation fuzzing can be done as described here

Implement the PreprocessTxs ABCI method using WirePayForMessage

After defining WirePayForMessage and connecting it the rest of the application, the draft implementation for PreprocessTxs needs to:

1. cleanly separate share messages from other transactions
2. perform basic validation on WirePayForMessages
3. pick block size (fixed for now)
4. remove unused share commitments

There are still some design decisions that need to be made. Notably:

1. How are we handling errors in PreprocessTxs? The ResponsePreprocessTxs only expects txs and messages.
2. Are we burning fees the same way most ethereum projects do? Send to the zero address?
3. Should the fees actually get burned in PreprocessTxs or in DeliverTxs?
4. The spec has a set of enums for the transaction types, so I think that means that each transaction goes through the same endpoint, not unlike ethereum, but the sdk expects typed rpc calls. Thoughts?
5. I'm guessing that block size and fees should be static for the very first draft, but in the future, is that going to be handled by core or the app?
6. Same goes for namespace size. Constant? If not, who decides, the app or core?
7. Should we be using big.Int instead of uint64?

Remove Redundant Signature Verification in `WirePayForMessage`

Currently, the signature for WirePayForMessage is being validated in WirePayForMessage's ValidateBasic method using the public key of the sender. This is unnecessary, because the signature is already being verified by the auth module's sigverify antehandler, and undesirable because it requires the 33 byte public key be included in each WirePayForMessage. Instead the signature verification in ValidateBasic should be removed, and only the sender's address (20 bytes) should be included in WirePayForMessage.

EPIC: Define and Implement Blob Inclusion Fraud Proofs

Summary

Implement MessageInclusionFraudProofs and add them to the spec.

Details

Message inclusion fraud proofs are needed to ensure that when a SignedTransactionDataPayForMessage (STDPfM) is included in a block, then the message that it pays for is also included in the block data.

The fraud proof can be constructed using the WrappedTransaction of the STDPfM that does not have a message included in the block data. The position and the number of shares used by the message are used to locate the subtree root for that message. Then an nmt merkle proof can be generated for that sub tree root (aka commitment to the message). If the merkle root the merkle proof is not equal to it's respective column or row root in the DataAvailabilityHeader, then the fraud proof is valid, and the block is invalid.

The position is included in the WrappedTransaction, and the number of shares used by the message can be calculated using the ShareSize constant and the messageSize field found in the STDPfM.

Action Items

• Define MessageInclusionFraudProof and add it to the spec
• Write function to create a MessageInclusionFraudProof
• Write a function to verify a MessageInclusionFraudProof
• Incorporate the fraud proofs into the celestia-node

References

blocked by celestiaorg/celestia-core#462
tracked in celestiaorg/celestia-core#459

docker: make a docker image for the app

Summary

We need to create a docker image for the celestia-app that handles all installation inside

Problem Definition

We don't have a docker image for the repo except for the contrib dir part

Proposal

To make an image that

• provides a ready to execute package with default settings, if not set in env vars
• have an entry.sh file that handles initialisation part
• triggers through a make file option

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

Bound shares used for transactions

The number of shares used for transactions, intermediate state roots, evidence, etc. should be bounded by some parameter. Otherwise, a malicious validator could create a maximum-size block filled with only transactions, and all full node would be required to fully download and process the block to compute the state.

specs: Document support for native multisigs

Currently, we only support externally-owned accounts (EOAs), i.e. accounts that are controlled by a single secret key.

We would like to natively support multisigs, i.e. accounts that are controlled by more than one secret key, since this offers more security for users and validators (similar to 2FA). A downside of supporting native multisigs is increase in state transition complexity.

EPIC: Fees: investigate and implement fee-burning

We want to implement eip-1559 style fee burning (but in the current tendermint deferred execution model).

This requires two parts:

1. Investigate how to achieve the above using the current execution model and within the existing ABCI methods (keeping in mind that abci++ is currently being spec'ed out). This needs to be summarized in an ADR and @adlerjohn volunteered to have a look into this.
2. Implement the fee-burning mechanism according to the above ADR in alignment with the specs.

Until then, we can probably just live with #48.

Properly manage the `SignedTrasnactionDataPayForMessage` handler

Due to an error on my part, the handler for SignedTransactionDataPayForMessage is not being properly incorporated into the app. To incorporate properly, the keeper package in the lazyledgerapp module should have its own MsgServer interface, that accounts for the additional method that does not have an endpoint.

Fix WirePayForMessage transaction generation when using cli

With the addition of #129, we started signing over the the entire sdk.Tx that contians the MsgPayForMessage instead of only the sdk.Message when we create MsgWirePayForMessages. This means that when we create the WirePayForMessage, we need to know everything that goes in the transaction for MsgPayForMessage will contain, such as gas price, sequence, etc, ahead of time before we sign it. This is the case when programmatically generating MsgWirePayForMessages using the client code (like we do in optimint), but this is not the case when generating MsgWirePayForMessages using the cli.

We should changes the way we generate and broadcast MsgWirePayForMessages when using the cli. Instead of using the default cosmos-sdk to BroadcastTxs, which parses things like gas-price, sequence, etc after generating the MsgWirePayForMessage, we should parse the cli flags/args manually and create the tx using the client code before signing the commitments.

Implement PayForMessage client code

Some client code for for signing and broadcasting WirePayForMessages was implemented in #76, and per the suggestion in this comment rollkit/rollkit#76 (comment) , we should keep that code here. It might also be nice to change up the API slightly to be a tad more general, and maybe even reuse that code in the already implemented cli.

clean up imports

I quickly skimmed through the imports and it seems that the app imports a mixture of lazyledger-core and github.com/tendermint/tendermint.

Start creating versioned docker images

Most of the devops tests from gaia and the cosmos-sdk could be used in lazyledger-app, but a few had to be temporarily removed because they are meant to use versioned docker images. When we start versioning, we should get a docker hub account for lazyledger, and begin creating docker images for each release.

ref #3 #8

Add reserved namespace ID for padding

Taking some inspiration from this post's suggestion of

Application ID 0 is interpreted as an empty entry and can be inserted anywhere by the block producers; it might be required for padding. Any application ID 0 fields should be ignored by clients.

We could use the application ID 0x0000000000000000 to denote any padding shares, then modify the rules of the NMT to simply ignore such shares when computing the min and max namespace IDs of a subtree. Note that the NMT already has special rules to handle tail padding, though since tail padding has the maximum namespace ID it is guaranteed to be lexicographically sorted. These padding shares with a zero namespace ID would not be.

The intuitions around why this should be possible is that there is no need to use the lexicographic-less-than binary relation specifically. The binary relation used for ordering namespace IDs does not necessarily need to be transitive or antisymmetric for its entire domain. As such, we can simply define a new one that ignores the zero namespace ID (i.e. is true if one or more of the inputs to the relation is the zero namespace ID).

The reason this would be useful is that currently padding between messages cannot be distinguished from messages. This means applications must potentially download a bunch of extra data that isn't useful for them. If padding has a distinct namespace ID, then it can be distinguished and simply not downloaded.

address: change length to 32

Summary

Addresses are currently of length 20. there was discussion in discord to migrate to 32 byte addresses. This would include nodeID's as well.

This is also outlined in the spec as the favored approach.

Update to patched version of tendermint/spm

In order to update to the latest release version of the cosmos-sdk, we need to use a patched version of tendermint/spm due to #10226 cosmos/cosmos-sdk@bdc0ca0 removing the ReadHomeFlag function

blocked by: #18 #19

specs: consensus leader selection & fork choice

Summary

Currently, the specs do not define the concrete Leader selection and the Fork Choice rule:
https://github.com/celestiaorg/celestia-specs/blob/06be92043f8ec39bab4222708c0fb44eb7fbca60/src/specs/consensus.md#fork-choice

IMO, these could be very short paragraphs (one or two sentences) that give an overview and refer to tendermint for the details. If there is anything LL specific to consider here (e.g. any difference that stems from the minority assumption), it should also be mentioned somewhere.

Sub-Tasks

We also need to consider:

• timeouts
• data becoming available after the fact
• erasure coding fraud proofs
• fork choice with DAS (ref: celestiaorg/celestia-core#321, celestiaorg/celestia-specs#179)

partly includes: celestiaorg/celestia-node#1134

Rename to celestia-app

We should rename the app to keep up with the rest of the rebranded repos.

• Wait for lazyledger-core and the cosmos-sdk to be renamed
• Update imports to reflect rebrand
• #54

blocked by #26 and #419

Specify max block size and other parameters

The specification must include all system-wide parameters. Client side parameters should also be specified in the sense that there should be a recommendation for defaults and where applicable it should specify which ranges do make sense.

ref: #732

Add a way to generate a `MsgWirePayForMessage` from the cli

In the future, there will likely be multiple libraries and methods to generate WirePayForMessage, but currently there are none. I think a good starting point is to do so using a cli. The cli for lazyledger-appd is using the default starport scaffolding, and luckily the starport team has made it easy to add sub-commands to the existing code.

The first draft of the cli will probably look something like this, but more details will be in the PR

lazyledgerappd tx lazyledgerapp create-PayForMessage [hexNamespace] [hexMessage] [flags]

clarify gas metering

Summary

Clarify how to meter for gas for messages and state-relevant Txs too.

Context

Dropping some raw notes from discussions we had:

Transaction cost metering should only be based on bytes .
potenial workarounds: implicitly set gas limit to infinity (maybe not infinity, maybe use gas used purely for byte cost). This means no need for gas limit field in tx.

Some context:
In the SDK applications can define gas metering as they please.

Here is an example of metering based on bytes: https://github.com/cosmos/cosmos-sdk/blob/eb8aaf939513d3bf6e82bd13691cfbe9ef4ad28c/x/auth/ante/basic.go#L88-L95

more docs:
https://docs.cosmos.network/master/basics/gas-fees.html#block-gas-meter
and the AnteHandler: https://docs.cosmos.network/master/basics/gas-fees.html#antehandler

Timeline / Action Items

Note the implementation can happen for the incentivized testnet but we should definitely "de-risk" this earlier, i.e. we should start looking into what exact changes will be necessary certainly before the test-net milsetone.