diff --git a/go/consensus/cometbft/apps/governance/transactions.go b/go/consensus/cometbft/apps/governance/transactions.go index 1dedec8531b..53c951440a6 100644 --- a/go/consensus/cometbft/apps/governance/transactions.go +++ b/go/consensus/cometbft/apps/governance/transactions.go @@ -26,8 +26,13 @@ func (app *governanceApplication) submitProposal( "proposal_content", proposalContent, ) + params, err := state.ConsensusParameters(ctx) + if err != nil { + return nil, fmt.Errorf("failed to fetch consensus parameters: %w", err) + } + // Validate proposal content basics. - if err := proposalContent.ValidateBasic(); err != nil { + if err := proposalContent.ValidateBasic(params); err != nil { ctx.Logger().Debug("governance: malformed proposal content", "content", proposalContent, "err", err, @@ -39,11 +44,6 @@ func (app *governanceApplication) submitProposal( return nil, nil } - params, err := state.ConsensusParameters(ctx) - if err != nil { - return nil, fmt.Errorf("failed to fetch consensus parameters: %w", err) - } - // To not violate the consensus, change parameters proposals should be ignored when disabled. if proposalContent.ChangeParameters != nil && !params.EnableChangeParametersProposal { return nil, governance.ErrInvalidArgument diff --git a/go/consensus/cometbft/apps/governance/transactions_test.go b/go/consensus/cometbft/apps/governance/transactions_test.go index 860f1ddd513..90c5344a625 100644 --- a/go/consensus/cometbft/apps/governance/transactions_test.go +++ b/go/consensus/cometbft/apps/governance/transactions_test.go @@ -58,6 +58,9 @@ func TestSubmitProposal(t *testing.T) { VotingPeriod: beacon.EpochTime(50), } + allowPropMetaParams := *baseConsParams + allowPropMetaParams.AllowProposalMetadata = true + for _, tc := range []struct { msg string params *governance.ConsensusParameters @@ -157,6 +160,36 @@ func TestSubmitProposal(t *testing.T) { func() {}, nil, }, + { + "should fail with metadata when metadata is not allowed", + baseConsParams, + pk1, + &governance.ProposalContent{ + Metadata: &governance.ProposalMetadata{ + Title: "Proposal that should fail", + }, + Upgrade: &governance.UpgradeProposal{ + Descriptor: baseAtEpoch(200), + }, + }, + func() {}, + governance.ErrInvalidArgument, + }, + { + "should work with metadata when metadata is allowed", + &allowPropMetaParams, + pk1, + &governance.ProposalContent{ + Metadata: &governance.ProposalMetadata{ + Title: "Proposal that should work", + }, + Upgrade: &governance.UpgradeProposal{ + Descriptor: baseAtEpoch(200), + }, + }, + func() {}, + nil, + }, { "should work with valid cancel upgrade proposal", baseConsParams, diff --git a/go/consensus/cometbft/apps/supplementarysanity/checks.go b/go/consensus/cometbft/apps/supplementarysanity/checks.go index 74d7112b034..81cf99f8aba 100644 --- a/go/consensus/cometbft/apps/supplementarysanity/checks.go +++ b/go/consensus/cometbft/apps/supplementarysanity/checks.go @@ -322,7 +322,7 @@ func checkGovernance(ctx *abciAPI.Context, epoch beacon.EpochTime) error { if err != nil { return fmt.Errorf("Proposals(): %w", err) } - err = governance.SanityCheckProposals(proposals, epoch, govDeposits) + err = governance.SanityCheckProposals(proposals, epoch, govDeposits, params) if err != nil { return fmt.Errorf("SanityCheck Proposals: %w", err) } diff --git a/go/governance/api/api.go b/go/governance/api/api.go index b59fd3ac7aa..01f51a66bec 100644 --- a/go/governance/api/api.go +++ b/go/governance/api/api.go @@ -7,7 +7,6 @@ import ( "encoding/base64" "fmt" "io" - "reflect" beacon "github.com/oasisprotocol/oasis-core/go/beacon/api" "github.com/oasisprotocol/oasis-core/go/common/cbor" @@ -60,21 +59,38 @@ var ( // ProposalContent is a consensus layer governance proposal content. type ProposalContent struct { + // Metadata contains optional proposal metadata which is ignored during proposal execution. + Metadata *ProposalMetadata `json:"metadata,omitempty"` + Upgrade *UpgradeProposal `json:"upgrade,omitempty"` CancelUpgrade *CancelUpgradeProposal `json:"cancel_upgrade,omitempty"` ChangeParameters *ChangeParametersProposal `json:"change_parameters,omitempty"` } // ValidateBasic performs basic proposal content validity checks. -func (p *ProposalContent) ValidateBasic() error { - numProposals := 0 - values := reflect.ValueOf(*p) - for i := 0; i < values.NumField(); i++ { - if !values.Field(i).IsNil() { - numProposals++ +func (p *ProposalContent) ValidateBasic(params *ConsensusParameters) error { + // Validate metadata if present. + if p.Metadata != nil { + if !params.AllowProposalMetadata { + return ErrInvalidArgument // Must be consistent with error during unmarshal. + } + if err := p.Metadata.ValidateBasic(); err != nil { + return err } } + // Validate content. + var numProposals int + if p.Upgrade != nil { + numProposals++ + } + if p.CancelUpgrade != nil { + numProposals++ + } + if p.ChangeParameters != nil { + numProposals++ + } + switch { case numProposals > 1: return fmt.Errorf("proposal content has multiple fields set") @@ -121,6 +137,9 @@ func (p *ProposalContent) Equals(other *ProposalContent) bool { // PrettyPrint writes a pretty-printed representation of ProposalContent to the // given writer. func (p ProposalContent) PrettyPrint(ctx context.Context, prefix string, w io.Writer) { + if p.Metadata != nil { + p.Metadata.PrettyPrint(ctx, prefix, w) + } if p.Upgrade != nil { fmt.Fprintf(w, "%sUpgrade:\n", prefix) p.Upgrade.PrettyPrint(ctx, prefix+" ", w) @@ -141,6 +160,41 @@ func (p ProposalContent) PrettyType() (interface{}, error) { return p, nil } +// MaxProposalTitleLength is the maximum length of a proposal's title. +const MaxProposalTitleLength = 100 + +// ProposalMetadata contains metadata about a proposal. +type ProposalMetadata struct { + // Title is the human-readable proposal title. + Title string `json:"title"` + // Description is the human-readable description. + Description string `json:"description,omitempty"` +} + +// ValidateBasic performs basic proposal metadata validity checks. +func (p *ProposalMetadata) ValidateBasic() error { + if len(p.Title) > MaxProposalTitleLength { + return fmt.Errorf("%w: proposal title too long", ErrInvalidArgument) + } + return nil +} + +// PrettyPrint writes a pretty-printed representation of ProposalMetadata to the given writer. +func (p ProposalMetadata) PrettyPrint(ctx context.Context, prefix string, w io.Writer) { + if len(p.Title) > 0 { + fmt.Fprintf(w, "%sTitle: %s\n", prefix, p.Title) + } + if len(p.Description) > 0 { + fmt.Fprintf(w, "%sDescription:\n", prefix) + fmt.Fprintf(w, "%s%s\n", prefix, p.Description) + } +} + +// PrettyType returns a representation of ProposalMetadata that can be used for pretty printing. +func (p ProposalMetadata) PrettyType() (interface{}, error) { + return p, nil +} + // UpgradeProposal is an upgrade proposal. type UpgradeProposal struct { upgrade.Descriptor @@ -379,6 +433,9 @@ type ConsensusParameters struct { // AllowVoteWithoutEntity is true iff casting votes without a registered entity is allowed. AllowVoteWithoutEntity bool `json:"allow_vote_without_entity,omitempty"` + + // AllowProposalMetadata is true iff proposals are allowed to contain metadata. + AllowProposalMetadata bool `json:"allow_proposal_metadata,omitempty"` } // ConsensusParameterChanges are allowed governance consensus parameter changes. diff --git a/go/governance/api/sanity_check.go b/go/governance/api/sanity_check.go index 23486ee82c5..a767c2948a4 100644 --- a/go/governance/api/sanity_check.go +++ b/go/governance/api/sanity_check.go @@ -48,7 +48,7 @@ func (c *ConsensusParameterChanges) SanityCheck() error { } // SanityCheckProposals sanity checks proposals. -func SanityCheckProposals(proposals []*Proposal, epoch beacon.EpochTime, governanceDeposit *quantity.Quantity) error { +func SanityCheckProposals(proposals []*Proposal, epoch beacon.EpochTime, governanceDeposit *quantity.Quantity, params *ConsensusParameters) error { activeProposalDeposits := quantity.NewFromUint64(0) for _, p := range proposals { if p.CreatedAt > epoch { @@ -57,7 +57,7 @@ func SanityCheckProposals(proposals []*Proposal, epoch beacon.EpochTime, governa if !p.Submitter.IsValid() { return fmt.Errorf("proposal %v: invalid proposal submitter", p.ID) } - if err := p.Content.ValidateBasic(); err != nil { + if err := p.Content.ValidateBasic(params); err != nil { return fmt.Errorf("proposal %v: basic validation failure: %w", p.ID, err) } @@ -146,7 +146,7 @@ func (g *Genesis) SanityCheck(now beacon.EpochTime, governanceDeposits *quantity if err := g.Parameters.SanityCheck(); err != nil { return fmt.Errorf("governance: parameters sanity check failed: %w", err) } - if err := SanityCheckProposals(g.Proposals, now, governanceDeposits); err != nil { + if err := SanityCheckProposals(g.Proposals, now, governanceDeposits, &g.Parameters); err != nil { return fmt.Errorf("governance: proposals sanity check failed: %w", err) } for _, p := range g.Proposals { diff --git a/go/oasis-test-runner/scenario/e2e/upgrade.go b/go/oasis-test-runner/scenario/e2e/upgrade.go index b777111c142..892cb0c1c77 100644 --- a/go/oasis-test-runner/scenario/e2e/upgrade.go +++ b/go/oasis-test-runner/scenario/e2e/upgrade.go @@ -108,6 +108,9 @@ func (c *upgrade240Checker) PreUpgradeFn(ctx context.Context, ctrl *oasis.Contro if govParams.AllowVoteWithoutEntity { return fmt.Errorf("voting without entity is allowed") } + if govParams.AllowProposalMetadata { + return fmt.Errorf("proposal metadata is allowed") + } return nil } @@ -157,6 +160,9 @@ func (c *upgrade240Checker) PostUpgradeFn(ctx context.Context, ctrl *oasis.Contr if !govParams.AllowVoteWithoutEntity { return fmt.Errorf("voting without entity is not allowed") } + if !govParams.AllowProposalMetadata { + return fmt.Errorf("proposal metadata is not allowed") + } return nil } diff --git a/go/upgrade/migrations/consensus_240.go b/go/upgrade/migrations/consensus_240.go index a4b731148a2..9234f8ae38b 100644 --- a/go/upgrade/migrations/consensus_240.go +++ b/go/upgrade/migrations/consensus_240.go @@ -79,6 +79,7 @@ func (h *Handler240) ConsensusUpgrade(privateCtx interface{}) error { return fmt.Errorf("failed to load governance consensus parameters: %w", err) } govParams.AllowVoteWithoutEntity = true + govParams.AllowProposalMetadata = true if err = govState.SetConsensusParameters(abciCtx, govParams); err != nil { return fmt.Errorf("failed to update governance consensus parameters: %w", err)