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.

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

lib/babe: always use 2/3 of slot to produce block, re-add potentially valid txs to queue #1679

Merged
merged 19 commits into from
Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
17 changes: 12 additions & 5 deletions lib/babe/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,6 @@ func (b *BlockBuilder) buildBlockExtrinsics(slot Slot) []*transaction.ValidTrans
txn := b.transactionState.Pop()
// Transaction queue is empty.
if txn == nil {
return included
}

// Move to next extrinsic.
if txn.Extrinsic == nil {
continue
}

Expand All @@ -270,6 +265,18 @@ func (b *BlockBuilder) buildBlockExtrinsics(slot Slot) []*transaction.ValidTrans
if _, ok := err.(*DispatchOutcomeError); !ok {
continue
}

// don't drop transactions that may be valid in a later block ie.
// run out of gas for this block or have a nonce that may be valid in a later block
if tvErr, ok := err.(*TransactionValidityError); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use errors.As(err, &e) https://blog.golang.org/go1.13-errors

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove indentation

	tvErr, ok := err.(*TransactionValidityError)
	if !ok {
		continue
	}
	switch tvErr.msg {
	case exhaustsResourcesMsg, invalidTransactionMsg:
		hash, err := b.transactionState.Push(txn)
		if err != nil {
			logger.Debug("failed to re-add transaction to queue", "tx", hash, "error", err)
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

switch tvErr.msg {
case exhaustsResourcesMsg, invalidTransactionMsg:
hash, err := b.transactionState.Push(txn)
if err != nil {
logger.Debug("failed to re-add transaction to queue", "tx", hash, "error", err)
}
}
}
}

logger.Debug("build block applied extrinsic", "extrinsic", extrinsic)
Expand Down
48 changes: 34 additions & 14 deletions lib/babe/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,33 @@ func (e DispatchOutcomeError) Error() string {

// A TransactionValidityError is possible errors while checking the validity of a transaction
type TransactionValidityError struct {
msg string // description of error
msg transactionValidityErrorMsg // description of error
}

func (e TransactionValidityError) Error() string {
return fmt.Sprintf("transaction validity error: %s", e.msg)
}

type transactionValidityErrorMsg string
Copy link
Member

Choose a reason for hiding this comment

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

why this type is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that not any string can be used as the error message, only the predefined ones. I can remove it if you think it's overkill

Copy link
Member

Choose a reason for hiding this comment

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

Got it! IMO seems overkill, I think that each of the errors string var could be of the type error like that:

var (
  ErrunexpectedTxCallMsg  = errors.New("call of the transaction is not expected")
  ErrinvalidPaymentMsg  = errors.New("invalid payment")
  ...
)

then, will be easy to determine a if an error occurs:

err := determineErrType(...)
if errors.Is(err, ErrunexpectedTxCallMsg) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the errors!


var (
unexpectedTxCallMsg transactionValidityErrorMsg = "call of the transaction is not expected"
invalidPaymentMsg transactionValidityErrorMsg = "invalid payment"
invalidTransactionMsg transactionValidityErrorMsg = "invalid transaction"
outdatedTransactionMsg transactionValidityErrorMsg = "outdated transaction"
badProofMsg transactionValidityErrorMsg = "bad proof"
ancientBirthBlockMsg transactionValidityErrorMsg = "ancient birth block"
exhaustsResourcesMsg transactionValidityErrorMsg = "exhausts resources"
mandatoryDispatchErrorMsg transactionValidityErrorMsg = "mandatory dispatch error"
invalidMandatoryDispatchMsg transactionValidityErrorMsg = "invalid mandatory dispatch"
lookupFailedMsg transactionValidityErrorMsg = "lookup failed"
validatorNotFoundMsg transactionValidityErrorMsg = "validator not found"
)

func newUnknownErrorMsg(data scale.VaryingDataTypeValue) transactionValidityErrorMsg {
return transactionValidityErrorMsg(fmt.Sprintf("unknown error: %d", data))
}

// UnmarshalError occurs when unmarshalling fails
type UnmarshalError struct {
msg string
Expand Down Expand Up @@ -223,31 +243,31 @@ func determineErrType(vdt scale.VaryingDataType) error {
case Module:
return &DispatchOutcomeError{fmt.Sprintf("custom module error: %s", val.string())}
case Call:
return &TransactionValidityError{"call of the transaction is not expected"}
return &TransactionValidityError{unexpectedTxCallMsg}
case Payment:
return &TransactionValidityError{"invalid payment"}
return &TransactionValidityError{invalidPaymentMsg}
case Future:
return &TransactionValidityError{"invalid transaction"}
return &TransactionValidityError{invalidTransactionMsg}
case Stale:
return &TransactionValidityError{"outdated transaction"}
return &TransactionValidityError{outdatedTransactionMsg}
case BadProof:
return &TransactionValidityError{"bad proof"}
return &TransactionValidityError{badProofMsg}
case AncientBirthBlock:
return &TransactionValidityError{"ancient birth block"}
return &TransactionValidityError{ancientBirthBlockMsg}
case ExhaustsResources:
return &TransactionValidityError{"exhausts resources"}
return &TransactionValidityError{exhaustsResourcesMsg}
case InvalidCustom:
return &TransactionValidityError{fmt.Sprintf("unknown error: %d", val)}
return &TransactionValidityError{newUnknownErrorMsg(val)}
case BadMandatory:
return &TransactionValidityError{"mandatory dispatch error"}
return &TransactionValidityError{mandatoryDispatchErrorMsg}
case MandatoryDispatch:
return &TransactionValidityError{"invalid mandatory dispatch"}
return &TransactionValidityError{invalidMandatoryDispatchMsg}
case ValidityCannotLookup:
return &TransactionValidityError{"lookup failed"}
return &TransactionValidityError{lookupFailedMsg}
case NoUnsignedValidator:
return &TransactionValidityError{"validator not found"}
return &TransactionValidityError{validatorNotFoundMsg}
case UnknownCustom:
return &TransactionValidityError{fmt.Sprintf("unknown error: %d", val)}
return &TransactionValidityError{newUnknownErrorMsg(val)}
}

return errInvalidResult
Expand Down
2 changes: 1 addition & 1 deletion lib/babe/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func TestApplyExtrinsicErrors(t *testing.T) {
_, ok := err.(*TransactionValidityError)
require.True(t, ok)
}
require.Equal(t, err.Error(), c.expected)
require.Equal(t, c.expected, err.Error())
})
}
}