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.

Sign up for GitHub

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

feat: support v3 app #3870

Merged
merged 10 commits into from
Sep 17, 2024
Merged

feat: support v3 app #3870

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cdcc56e
support v3 app
cmwaters Sep 12, 2024
2025907
add a test to upgrade to v3
cmwaters Sep 13, 2024
e666ddd
fix share versioning support
cmwaters Sep 13, 2024
d4dd518
Merge branch 'main' into cal/v3
cmwaters Sep 16, 2024
122c111
fix tests
cmwaters Sep 16, 2024
76ef9f3
version ibc modules
cmwaters Sep 16, 2024
b1f3223
Apply suggestions from code review
cmwaters Sep 16, 2024
3ce4e3c
use the official v2.0.0 go-square release
cmwaters Sep 16, 2024
5536bfc
Merge branch 'main' into cal/v3
cmwaters Sep 17, 2024
3b6ff9d
Merge branch 'main' into cal/v3
cmwaters Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/celestiaorg/celestia-app/v3/app/posthandler"
appv1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1"
appv2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
appv3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3"
"github.com/celestiaorg/celestia-app/v3/pkg/proof"
blobkeeper "github.com/celestiaorg/celestia-app/v3/x/blob/keeper"
blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types"
Expand Down Expand Up @@ -108,6 +109,7 @@ var maccPerms = map[string][]string{
const (
v1 = appv1.Version
v2 = appv2.Version
v3 = appv3.Version
DefaultInitialVersion = v1
)

Expand Down Expand Up @@ -341,10 +343,10 @@ func New(
packetforwardkeeper.DefaultRefundTransferPacketTimeoutTimestamp, // refund timeout
)
// PacketForwardMiddleware is used only for version 2.
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
transferStack = module.NewVersionedIBCModule(packetForwardMiddleware, transferStack, v2, v2)
transferStack = module.NewVersionedIBCModule(packetForwardMiddleware, transferStack, v2, v3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have some kind of test to avoid forgetting to bump versioned ibc modules?

// Token filter wraps packet forward middleware and is thus the first module in the transfer stack.
tokenFilterMiddelware := tokenfilter.NewIBCMiddleware(transferStack)
transferStack = module.NewVersionedIBCModule(tokenFilterMiddelware, transferStack, v1, v2)
transferStack = module.NewVersionedIBCModule(tokenFilterMiddelware, transferStack, v1, v3)

app.EvidenceKeeper = *evidencekeeper.NewKeeper(
appCodec,
Expand Down
5 changes: 2 additions & 3 deletions app/check_tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
switch req.Type {
// new transactions must be checked in their entirety
case abci.CheckTxType_New:
// FIXME: we have a hardcoded subtree root threshold here. This is because we can't access
// the app version because the context is not initialized
err := blobtypes.ValidateBlobTx(app.txConfig, btx, appconsts.DefaultSubtreeRootThreshold)
appVersion := app.AppVersion()
err := blobtypes.ValidateBlobTx(app.txConfig, btx, appconsts.SubtreeRootThreshold(appVersion), appVersion)
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, []abci.Event{}, false)
}
Expand Down
2 changes: 1 addition & 1 deletion app/module/versioned_ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func TestVersionedIBCModule(t *testing.T) {
mockWrappedModule := mocks.NewMockIBCModule(ctrl)
mockNextModule := mocks.NewMockIBCModule(ctrl)

versionedModule := module.NewVersionedIBCModule(mockWrappedModule, mockNextModule, 2, 2)
versionedModule := module.NewVersionedIBCModule(mockWrappedModule, mockNextModule, 2, 3)

testCases := []struct {
name string
Expand Down
68 changes: 44 additions & 24 deletions app/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,95 +96,95 @@ func (app *App) setupModuleManager(skipGenesisInvariants bool) error {
app.manager, err = module.NewManager([]module.VersionedModule{
{
Module: genutil.NewAppModule(app.AccountKeeper, app.StakingKeeper, app.BaseApp.DeliverTx, app.txConfig),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: auth.NewAppModule(app.appCodec, app.AccountKeeper, nil),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: vesting.NewAppModule(app.AccountKeeper, app.BankKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: bank.NewAppModule(app.appCodec, app.BankKeeper, app.AccountKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: capability.NewAppModule(app.appCodec, *app.CapabilityKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: feegrantmodule.NewAppModule(app.appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: crisis.NewAppModule(&app.CrisisKeeper, skipGenesisInvariants),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: gov.NewAppModule(app.appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: mint.NewAppModule(app.appCodec, app.MintKeeper, app.AccountKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: slashing.NewAppModule(app.appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: distr.NewAppModule(app.appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: staking.NewAppModule(app.appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: evidence.NewAppModule(app.EvidenceKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: authzmodule.NewAppModule(app.appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: ibc.NewAppModule(app.IBCKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: params.NewAppModule(app.ParamsKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: transfer.NewAppModule(app.TransferKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: blob.NewAppModule(app.appCodec, app.BlobKeeper),
FromVersion: v1, ToVersion: v2,
FromVersion: v1, ToVersion: v3,
},
{
Module: blobstream.NewAppModule(app.appCodec, app.BlobstreamKeeper),
FromVersion: v1, ToVersion: v1,
},
{
Module: signal.NewAppModule(app.SignalKeeper),
FromVersion: v2, ToVersion: v2,
FromVersion: v2, ToVersion: v3,
},
{
Module: minfee.NewAppModule(app.ParamsKeeper),
FromVersion: v2, ToVersion: v2,
FromVersion: v2, ToVersion: v3,
},
{
Module: packetforward.NewAppModule(app.PacketForwardKeeper),
FromVersion: v2, ToVersion: v2,
FromVersion: v2, ToVersion: v3,
},
{
Module: ica.NewAppModule(nil, &app.ICAHostKeeper),
FromVersion: v2, ToVersion: v2,
FromVersion: v2, ToVersion: v3,
},
})
if err != nil {
Expand Down Expand Up @@ -303,7 +303,7 @@ func allStoreKeys() []string {
// versionedStoreKeys returns the store keys for each app version.
func versionedStoreKeys() map[uint64][]string {
return map[uint64][]string{
1: {
v1: {
authtypes.StoreKey,
authzkeeper.StoreKey,
banktypes.StoreKey,
Expand All @@ -321,7 +321,7 @@ func versionedStoreKeys() map[uint64][]string {
stakingtypes.StoreKey,
upgradetypes.StoreKey,
},
2: {
v2: {
authtypes.StoreKey,
authzkeeper.StoreKey,
banktypes.StoreKey,
Expand All @@ -341,6 +341,26 @@ func versionedStoreKeys() map[uint64][]string {
stakingtypes.StoreKey,
upgradetypes.StoreKey,
},
v3: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[no change needed] verified this is the exact same list as v2 https://www.diffchecker.com/pHyajXyz/

authtypes.StoreKey,
authzkeeper.StoreKey,
banktypes.StoreKey,
blobtypes.StoreKey,
capabilitytypes.StoreKey,
distrtypes.StoreKey,
evidencetypes.StoreKey,
feegrant.StoreKey,
govtypes.StoreKey,
ibchost.StoreKey,
ibctransfertypes.StoreKey,
icahosttypes.StoreKey,
minttypes.StoreKey,
packetforwardtypes.StoreKey,
signaltypes.StoreKey,
slashingtypes.StoreKey,
stakingtypes.StoreKey,
upgradetypes.StoreKey,
},
}
}

Expand Down
40 changes: 32 additions & 8 deletions app/prepare_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package app

import (
"fmt"
"time"

"github.com/celestiaorg/celestia-app/v3/app/ante"
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v3/pkg/da"
square "github.com/celestiaorg/go-square/v2"
"github.com/celestiaorg/go-square/v2/share"
shares "github.com/celestiaorg/go-square/shares"
square "github.com/celestiaorg/go-square/square"
squarev2 "github.com/celestiaorg/go-square/v2"
sharev2 "github.com/celestiaorg/go-square/v2/share"
"github.com/cosmos/cosmos-sdk/telemetry"
abci "github.com/tendermint/tendermint/abci/types"
core "github.com/tendermint/tendermint/proto/tendermint/types"
Expand All @@ -27,7 +30,7 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
Height: req.Height,
Time: req.Time,
Version: version.Consensus{
App: app.BaseApp.AppVersion(),
App: app.AppVersion(),
},
})
handler := ante.NewAnteHandler(
Expand All @@ -47,18 +50,39 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr

// Build the square from the set of valid and prioritised transactions.
// The txs returned are the ones used in the square and block.
dataSquare, txs, err := square.Build(txs,
app.MaxEffectiveSquareSize(sdkCtx),
appconsts.SubtreeRootThreshold(app.GetBaseApp().AppVersion()),
var (
dataSquareBytes [][]byte
err error
size uint64
)
switch app.AppVersion() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a unit test to prepare proposal to verify this logic?

case v3:
var dataSquare squarev2.Square
dataSquare, txs, err = squarev2.Build(txs,
app.MaxEffectiveSquareSize(sdkCtx),
appconsts.SubtreeRootThreshold(app.GetBaseApp().AppVersion()),
)
dataSquareBytes = sharev2.ToBytes(dataSquare)
size = uint64(dataSquare.Size())
case v2, v1:
var dataSquare square.Square
dataSquare, txs, err = square.Build(txs,
app.MaxEffectiveSquareSize(sdkCtx),
appconsts.SubtreeRootThreshold(app.GetBaseApp().AppVersion()),
)
dataSquareBytes = shares.ToBytes(dataSquare)
size = uint64(dataSquare.Size())
default:
err = fmt.Errorf("unsupported app version: %d", app.AppVersion())
}
if err != nil {
panic(err)
}

// Erasure encode the data square to create the extended data square (eds).
// Note: uses the nmt wrapper to construct the tree. See
// pkg/wrapper/nmt_wrapper.go for more information.
eds, err := da.ExtendShares(share.ToBytes(dataSquare))
eds, err := da.ExtendShares(dataSquareBytes)
if err != nil {
app.Logger().Error(
"failure to erasure the data square while creating a proposal block",
Expand All @@ -84,7 +108,7 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
return abci.ResponsePrepareProposal{
BlockData: &core.Data{
Txs: txs,
SquareSize: uint64(dataSquare.Size()),
SquareSize: size,
Hash: dah.Hash(), // also known as the data root
},
}
Expand Down
49 changes: 34 additions & 15 deletions app/process_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import (
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v3/pkg/da"
blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types"
"github.com/celestiaorg/go-square/v2"
"github.com/celestiaorg/go-square/v2/share"

cmwaters marked this conversation as resolved.
Show resolved Hide resolved
shares "github.com/celestiaorg/go-square/shares"
square "github.com/celestiaorg/go-square/square"
squarev2 "github.com/celestiaorg/go-square/v2"
sharev2 "github.com/celestiaorg/go-square/v2/share"
blobtx "github.com/celestiaorg/go-square/v2/tx"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -108,7 +111,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
// - that the sizes match
// - that the namespaces match between blob and PFB
// - that the share commitment is correct
if err := blobtypes.ValidateBlobTx(app.txConfig, blobTx, subtreeRootThreshold); err != nil {
if err := blobtypes.ValidateBlobTx(app.txConfig, blobTx, subtreeRootThreshold, app.AppVersion()); err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, fmt.Sprintf("invalid blob tx %d", idx), err)
return reject()
}
Expand All @@ -122,24 +125,40 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp

}

// Construct the data square from the block's transactions
dataSquare, err := square.Construct(
req.BlockData.Txs,
app.MaxEffectiveSquareSize(sdkCtx),
subtreeRootThreshold,
var (
dataSquareBytes [][]byte
err error
)
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to compute data square from transactions:", err)

switch app.AppVersion() {
case v3:
var dataSquare squarev2.Square
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a unit test to process proposal to verify this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What logic would this test be verifying? Do we want to build a v3 square with an authored blob and assert that we can parse out the signer or something like that?

dataSquare, err = squarev2.Construct(req.BlockData.Txs, app.MaxEffectiveSquareSize(sdkCtx), subtreeRootThreshold)
dataSquareBytes = sharev2.ToBytes(dataSquare)
// Assert that the square size stated by the proposer is correct
if uint64(dataSquare.Size()) != req.BlockData.SquareSize {
logInvalidPropBlock(app.Logger(), req.Header, "proposed square size differs from calculated square size")
return reject()
}
case v2, v1:
var dataSquare square.Square
dataSquare, err = square.Construct(req.BlockData.Txs, app.MaxEffectiveSquareSize(sdkCtx), subtreeRootThreshold)
dataSquareBytes = shares.ToBytes(dataSquare)
// Assert that the square size stated by the proposer is correct
if uint64(dataSquare.Size()) != req.BlockData.SquareSize {
logInvalidPropBlock(app.Logger(), req.Header, "proposed square size differs from calculated square size")
return reject()
}
default:
logInvalidPropBlock(app.Logger(), req.Header, "unsupported app version")
return reject()
}

// Assert that the square size stated by the proposer is correct
if uint64(dataSquare.Size()) != req.BlockData.SquareSize {
logInvalidPropBlock(app.Logger(), req.Header, "proposed square size differs from calculated square size")
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to compute data square from transactions:", err)
return reject()
}

eds, err := da.ExtendShares(share.ToBytes(dataSquare))
eds, err := da.ExtendShares(dataSquareBytes)
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to erasure the data square", err)
return reject()
Expand Down
2 changes: 1 addition & 1 deletion app/test/std_sdk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() {
name: "signal a version change",
msgFunc: func() (msgs []sdk.Msg, signer string) {
valAccount := s.getValidatorAccount()
msg := signal.NewMsgSignalVersion(valAccount, 2)
msg := signal.NewMsgSignalVersion(valAccount, 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the test case with the PR changes.

The version number used in the test case (4) is higher than the new application version being introduced in this PR (v3). To accurately validate the changes, consider updating the version number to match the new version.

Apply this diff to align the test case:

-msg := signal.NewMsgSignalVersion(valAccount, 4)
+msg := signal.NewMsgSignalVersion(valAccount, 3)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
msg := signal.NewMsgSignalVersion(valAccount, 4)
msg := signal.NewMsgSignalVersion(valAccount, 3)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
msg := signal.NewMsgSignalVersion(valAccount, 4)
msg := signal.NewMsgSignalVersion(valAccount, 3)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] 4 seems incorrect b/c that version isn't supported by the state machine? Why not use 3 here like Nina suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because 3 is the current version. It would be weird to signal for the current version. What is more normal is to signal for the next version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do something like LatestVersion + 1 so we don't have to continually increment it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, thanks! No longer blocking.

4 or LatestVersion + 1 both work for me.

return []sdk.Msg{msg}, s.getValidatorName()
},
expectedCode: abci.CodeTypeOK,
Expand Down
Loading
Loading