Merge pull request #3675 from oasisprotocol/kostko/fix/beacon-e2e-tests

beacon: Transaction handling fixes

.changelog/3675.internal.md

@@ -0,0 +1 @@
+go/consensus: Add support for method priority

go/beacon/api/pvss.go

@@ -27,24 +27,9 @@ type PVSSParameters struct {
 	RevealInterval  int64 `json:"reveal_interval"`
 	TransitionDelay int64 `json:"transition_delay"`
 
-	GasCosts transaction.Costs `json:"gas_costs,omitempty"`
-
 	DebugForcedParticipants []signature.PublicKey `json:"debug_forced_participants,omitempty"`
 }
 
-const (
-	// GasOpPVSSCommit is the gas operation identifier for a commit.
-	GasOpPVSSCommit transaction.Op = "pvss_commit"
-	// GasOpPVSSReveal is the gas operation identifier for a reveal.
-	GasOpPVSSReveal transaction.Op = "pvss_reveal"
-)
-
-// DefaultGasCosts are the "default" gas costs for operations.
-var DefaultGasCosts = transaction.Costs{
-	GasOpPVSSCommit: 1000,
-	GasOpPVSSReveal: 1000,
-}
-
 // PVSSCommit is a PVSS commitment transaction payload.
 type PVSSCommit struct {
 	Epoch EpochTime `json:"epoch"`
@@ -53,6 +38,15 @@ type PVSSCommit struct {
 	Commit *pvss.Commit `json:"commit,omitempty"`
 }
 
+// Implements transaction.MethodMetadataProvider.
+func (pc PVSSCommit) MethodMetadata() transaction.MethodMetadata {
+	return transaction.MethodMetadata{
+		// Beacon transactions are critical to protocol operation. Since they can only be called
+		// by the block proposer this is safe to use.
+		Priority: transaction.MethodPriorityCritical,
+	}
+}
+
 // PVSSReveal is a PVSS reveal transaction payload.
 type PVSSReveal struct {
 	Epoch EpochTime `json:"epoch"`
@@ -61,6 +55,15 @@ type PVSSReveal struct {
 	Reveal *pvss.Reveal `json:"reveal,omitempty"`
 }
 
+// Implements transaction.MethodMetadataProvider.
+func (pr PVSSReveal) MethodMetadata() transaction.MethodMetadata {
+	return transaction.MethodMetadata{
+		// Beacon transactions are critical to protocol operation. Since they can only be called
+		// by the block proposer this is safe to use.
+		Priority: transaction.MethodPriorityCritical,
+	}
+}
+
 // RoundState is a PVSS round state.
 type RoundState uint8
 

go/consensus/api/transaction/transaction.go

@@ -221,6 +221,28 @@ func Sign(signer signature.Signer, tx *Transaction) (*SignedTransaction, error)
 // MethodSeparator is the separator used to separate backend name from method name.
 const MethodSeparator = "."
 
+// MethodPriority is the method handling priority.
+type MethodPriority uint8
+
+const (
+	// MethodPriorityNormal is the normal method priority.
+	MethodPriorityNormal = 0
+	// MethodPriorityCritical is the priority for methods critical to the protocol operation.
+	MethodPriorityCritical = 255
+)
+
+// MethodMetadata is the method metadata.
+type MethodMetadata struct {
+	Priority MethodPriority
+}
+
+// MethodMetadataProvider is the method metadata provider interface that can be implemented by
+// method body types to provide additional method metadata.
+type MethodMetadataProvider interface {
+	// MethodMetadata returns the method metadata.
+	MethodMetadata() MethodMetadata
+}
+
 // MethodName is a method name.
 type MethodName string
 
@@ -239,6 +261,23 @@ func (m MethodName) BodyType() interface{} {
 	return bodyType
 }
 
+// Metadata returns the method metadata.
+func (m MethodName) Metadata() MethodMetadata {
+	mp, ok := m.BodyType().(MethodMetadataProvider)
+	if !ok {
+		// Return defaults.
+		return MethodMetadata{
+			Priority: MethodPriorityNormal,
+		}
+	}
+	return mp.MethodMetadata()
+}
+
+// IsCritical returns true if the method is critical for the operation of the protocol.
+func (m MethodName) IsCritical() bool {
+	return m.Metadata().Priority == MethodPriorityCritical
+}
+
 // NewMethodName creates a new method name.
 //
 // Module and method pair must be unique. If they are not, this method

go/consensus/api/transaction/transaction_test.go

@@ -0,0 +1,28 @@
+package transaction
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+type testMethodBodyNormal struct {
+}
+
+type testMethodBodyCritical struct {
+}
+
+func (tb testMethodBodyCritical) MethodMetadata() MethodMetadata {
+	return MethodMetadata{
+		Priority: MethodPriorityCritical,
+	}
+}
+
+func TestMethodMetadata(t *testing.T) {
+	require := require.New(t)
+
+	methodNormal := NewMethodName("test", "Normal", testMethodBodyNormal{})
+	methodCritical := NewMethodName("test", "Critical", testMethodBodyCritical{})
+	require.False(methodNormal.IsCritical())
+	require.True(methodCritical.IsCritical())
+}

go/consensus/tendermint/abci/mux.go

@@ -393,6 +393,7 @@ func (mux *abciMux) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginB
 	} else {
 		mux.state.blockCtx.Set(api.GasAccountantKey{}, api.NewNopGasAccountant())
 	}
+	mux.state.blockCtx.Set(api.BlockProposerKey{}, req.Header.ProposerAddress)
 	// Create BeginBlock context.
 	ctx := mux.state.NewContext(api.ContextBeginBlock, mux.currentTime)
 	defer ctx.Close()
@@ -521,8 +522,21 @@ func (mux *abciMux) decodeTx(ctx *api.Context, rawTx []byte) (*transaction.Trans
 }
 
 func (mux *abciMux) processTx(ctx *api.Context, tx *transaction.Transaction, txSize int) error {
+	// Lookup method handler.
+	app := mux.appsByMethod[tx.Method]
+	if app == nil {
+		ctx.Logger().Error("unknown method",
+			"tx", tx,
+			"method", tx.Method,
+		)
+		return fmt.Errorf("mux: unknown method: %s", tx.Method)
+	}
+
 	// Pass the transaction through the fee handler if configured.
-	if txAuthHandler := mux.state.txAuthHandler; txAuthHandler != nil {
+	//
+	// Ignore fees for critical protocol methods to ensure they are processed in a block. Note that
+	// this relies on method handlers to prevent DoS.
+	if txAuthHandler := mux.state.txAuthHandler; txAuthHandler != nil && !tx.Method.IsCritical() {
 		if err := txAuthHandler.AuthenticateTx(ctx, tx); err != nil {
 			ctx.Logger().Debug("failed to authenticate transaction",
 				"tx", tx,
@@ -541,15 +555,6 @@ func (mux *abciMux) processTx(ctx *api.Context, tx *transaction.Transaction, txS
 	}
 
 	// Route to correct handler.
-	app := mux.appsByMethod[tx.Method]
-	if app == nil {
-		ctx.Logger().Error("unknown method",
-			"tx", tx,
-			"method", tx.Method,
-		)
-		return fmt.Errorf("mux: unknown method: %s", tx.Method)
-	}
-
 	ctx.Logger().Debug("dispatching",
 		"app", app.Name(),
 		"tx", tx,

go/consensus/tendermint/api/api.go

@@ -325,3 +325,13 @@ func (bsc *BaseServiceClient) DeliverEvent(ctx context.Context, height int64, tx
 func (bsc *BaseServiceClient) DeliverCommand(ctx context.Context, height int64, cmd interface{}) error {
 	return nil
 }
+
+// BlockProposerKey is the block context key for storing the block proposer address.
+type BlockProposerKey struct{}
+
+// NewDefault returns a new default value for the given key.
+func (bpk BlockProposerKey) NewDefault() interface{} {
+	// This should never be called as a block proposer must always be created by the application
+	// multiplexer.
+	panic("no proposer address in block context")
+}

go/consensus/tendermint/apps/beacon/backend_pvss.go

@@ -39,6 +39,14 @@ var (
 	validatorEntropyCtx = []byte("EkB-validator")
 )
 
+// beaconTransactionIncludedKey is the block context key for storing the beacon transaction
+// inclusion flag to make sure that only a single beacon transaction is included.
+type beaconTransactionIncludedKey struct{}
+
+func (bti beaconTransactionIncludedKey) NewDefault() interface{} {
+	return false
+}
+
 type backendPVSS struct {
 	app *beaconApplication
 }
@@ -511,7 +519,7 @@ func (impl *backendPVSS) initRound(
 	// Note: Because of the +1, applied to BlockHeight, it may be required
 	// to strategically subtract 1 from one of the three interval/delay
 	// parameters (eg: Commit/Reveal/Delay set to 20/10/4 results in
-	// transitions at blocsk 35, 70, 105, ...).
+	// transitions at blocks 35, 70, 105, ...).
 
 	height := ctx.BlockHeight() + 1 // Current height is ctx.BlockHeight() + 1
 	pvssState.CommitDeadline = height + params.PVSSParameters.CommitInterval
@@ -550,10 +558,32 @@ func (impl *backendPVSS) ExecuteTx(
 		// time out due to the processing overhead.
 		//
 		// In an ideal world, beacon tx-es shouldn't be gossiped to begin
-		// with (and be rejected if received), and each block should be
-		// limited to one beacon tx.
-		if ctx.IsCheckOnly() && !staking.NewAddress(ctx.TxSigner()).Equal(ctx.AppState().OwnTxSignerAddress()) {
-			return fmt.Errorf("beacon: rejecting non-local beacon tx: %s", ctx.TxSigner())
+		// with.
+		switch {
+		case ctx.IsCheckOnly():
+			// During CheckTx do the quick check and only accept own transactions.
+			if !staking.NewAddress(ctx.TxSigner()).Equal(ctx.AppState().OwnTxSignerAddress()) {
+				return fmt.Errorf("beacon: rejecting non-local beacon tx: %s", ctx.TxSigner())
+			}
+		case ctx.IsSimulation():
+			// No checks needed during local simulation.
+		default:
+			// During DeliverTx make sure that the transaction comes from the block proposer.
+			registryState := registryState.NewMutableState(ctx.State())
+			proposerAddr := ctx.BlockContext().Get(api.BlockProposerKey{}).([]byte)
+			proposerNodeID, err := registryState.NodeIDByConsensusAddress(ctx, proposerAddr)
+			if err != nil {
+				return fmt.Errorf("beacon: failed to resolve proposer node: %w", err)
+			}
+			if !ctx.TxSigner().Equal(proposerNodeID) {
+				return fmt.Errorf("beacon: rejecting beacon tx not from proposer (proposer: %s signer: %s)", proposerNodeID, ctx.TxSigner())
+			}
+
+			// Also make sure that there is only a single beacon transaction in a block.
+			if ctx.BlockContext().Get(beaconTransactionIncludedKey{}).(bool) {
+				return fmt.Errorf("beacon: rejecting multiple beacon txes per block")
+			}
+			ctx.BlockContext().Set(beaconTransactionIncludedKey{}, true)
 		}
 		return impl.doPVSSTx(ctx, state, params, tx)
 	case MethodSetEpoch:
@@ -580,18 +610,6 @@ func (impl *backendPVSS) doPVSSTx(
 		return fmt.Errorf("beacon: no PVSS state, round not in progress")
 	}
 
-	// Charge gas for this transaction.
-	var gasOp transaction.Op
-	switch tx.Method {
-	case beacon.MethodPVSSCommit:
-		gasOp = beacon.GasOpPVSSCommit
-	case beacon.MethodPVSSReveal:
-		gasOp = beacon.GasOpPVSSReveal
-	}
-	if err = ctx.Gas().UseGas(1, gasOp, params.PVSSParameters.GasCosts); err != nil {
-		return err
-	}
-
 	// Ensure the tx is from a current valid participant.
 	registryState := registryState.NewMutableState(ctx.State())
 	node, err := registryState.Node(ctx, ctx.TxSigner())

go/consensus/tendermint/apps/registry/state/state.go

@@ -187,19 +187,30 @@ func (s *ImmutableState) Node(ctx context.Context, id signature.PublicKey) (*nod
 	return &node, nil
 }
 
-// NodeByConsensusAddress looks up a specific node by its consensus address.
-func (s *ImmutableState) NodeByConsensusAddress(ctx context.Context, address []byte) (*node.Node, error) {
+// NodeIDByConsensusAddress looks up a specific node ID by its consensus address.
+//
+// If you need to get the actual node descriptor, use NodeByConsensusAddress instead.
+func (s *ImmutableState) NodeIDByConsensusAddress(ctx context.Context, address []byte) (signature.PublicKey, error) {
 	rawID, err := s.is.Get(ctx, nodeByConsAddressKeyFmt.Encode(address))
 	if err != nil {
-		return nil, abciAPI.UnavailableStateError(err)
+		return signature.PublicKey{}, abciAPI.UnavailableStateError(err)
 	}
 	if rawID == nil {
-		return nil, registry.ErrNoSuchNode
+		return signature.PublicKey{}, registry.ErrNoSuchNode
 	}
 
 	var id signature.PublicKey
 	if err := id.UnmarshalBinary(rawID); err != nil {
-		return nil, abciAPI.UnavailableStateError(err)
+		return signature.PublicKey{}, abciAPI.UnavailableStateError(err)
+	}
+	return id, nil
+}
+
+// NodeByConsensusAddress looks up a specific node by its consensus address.
+func (s *ImmutableState) NodeByConsensusAddress(ctx context.Context, address []byte) (*node.Node, error) {
+	id, err := s.NodeIDByConsensusAddress(ctx, address)
+	if err != nil {
+		return nil, err
 	}
 	return s.Node(ctx, id)
 }

go/oasis-node/cmd/debug/txsource/workload/queries.go

@@ -278,18 +278,7 @@ func (q *queries) doConsensusQueries(ctx context.Context, rng *rand.Rand, height
 	}
 
 	var txEvents []*results.Event
-	for txIdx, res := range txsWithRes.Results {
-		if res.IsSuccess() {
-			// Successful transaction should always produce events.
-			if len(res.Events) == 0 {
-				q.logger.Error("GetTransactionsWithResults successful transaction without events",
-					"transaction", txsWithRes.Transactions[txIdx],
-					"result", res,
-					"height", height,
-				)
-				return fmt.Errorf("GetTransactionsWithResults successful transaction result without events")
-			}
-		}
+	for _, res := range txsWithRes.Results {
 		txEvents = append(txEvents, res.Events...)
 	}
 	if err := q.sanityCheckTransactionEvents(ctx, height, txEvents); err != nil {

go/oasis-node/cmd/genesis/genesis.go

@@ -271,7 +271,6 @@ func doInitGenesis(cmd *cobra.Command, args []string) {
 			CommitInterval:          viper.GetInt64(CfgBeaconPVSSCommitInterval),
 			RevealInterval:          viper.GetInt64(CfgBeaconPVSSRevealInterval),
 			TransitionDelay:         viper.GetInt64(CfgBeaconPVSSTransitionDelay),
-			GasCosts:                beacon.DefaultGasCosts, // TODO: Make these configurable.
 			DebugForcedParticipants: forcedParticipants,
 		}
 	}

go/oasis-test-runner/scenario/e2e/runtime/runtime.go

@@ -209,9 +209,15 @@ func (sc *runtimeImpl) Fixture() (*oasis.NetworkFixture, error) {
 		},
 	}
 
-	epochInterval, _ := sc.Flags.GetInt64(cfgEpochInterval)
-	ff.Network.Beacon.InsecureParameters = &beacon.InsecureParameters{
-		Interval: epochInterval,
+	if epochInterval, _ := sc.Flags.GetInt64(cfgEpochInterval); epochInterval > 0 {
+		ff.Network.Beacon.InsecureParameters = &beacon.InsecureParameters{
+			Interval: epochInterval,
+		}
+		ff.Network.Beacon.PVSSParameters = &beacon.PVSSParameters{
+			CommitInterval:  epochInterval / 2,
+			RevealInterval:  (epochInterval / 2) - 4,
+			TransitionDelay: 4,
+		}
 	}
 
 	return ff, nil