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 17 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
20 changes: 15 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,21 @@ 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
var e *TransactionValidityError
if !errors.As(err, &e) {
continue
}

err = err.(*TransactionValidityError).msg
Copy link
Member

Choose a reason for hiding this comment

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

This cast is not necessary since we already change the err into TransactionValidityError on line 273

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't, the above lines do this:

			var e *TransactionValidityError
			if !errors.As(err, &e) {
				continue
			}

that doesn't cast it as a *TransactionValidityError afaik

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the errors.As doesn't cast the variable err, it gets the error and transform it to type *TransactionValidityError and sets to variable &e, without change the err variable, and returns true (or false if fails the type doesn't match). So basically it is possible to use e.msg without the cast on line 276

from: https://golang.org/pkg/errors/#As

func As(err error, target interface{}) bool

errors.As finds the first error in err's chain that matches target, and if so, sets target to that error value and returns true. Otherwise, it returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay makes sense, I updated it!

if errors.Is(err, errExhaustsResources) || errors.Is(err, errInvalidTransaction) {
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior Jul 9, 2021

Choose a reason for hiding this comment

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

Suggested change
if errors.Is(err, errExhaustsResources) || errors.Is(err, errInvalidTransaction) {
if errors.Is(e.msg, errExhaustsResources) || errors.Is(e.msg, errInvalidTransaction) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this is necessary?

Copy link
Member

Choose a reason for hiding this comment

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

it just changes to use the e.msg instead of err since e variable has the correct msg error

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

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
46 changes: 32 additions & 14 deletions lib/babe/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,31 @@ 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 error // description of error
}

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

var (
errUnexpectedTxCall = errors.New("call of the transaction is not expected")
errInvalidPayment = errors.New("invalid payment")
errInvalidTransaction = errors.New("invalid transaction")
errOutdatedTransaction = errors.New("outdated transaction")
errBadProof = errors.New("bad proof")
errAncientBirthBlock = errors.New("ancient birth block")
errExhaustsResources = errors.New("exhausts resources")
errMandatoryDispatchError = errors.New("mandatory dispatch error")
errInvalidMandatoryDispatch = errors.New("invalid mandatory dispatch")
errLookupFailed = errors.New("lookup failed")
errValidatorNotFound = errors.New("validator not found")
)

func newUnknownError(data scale.VaryingDataTypeValue) error {
return fmt.Errorf("unknown error: %d", data)
}

// UnmarshalError occurs when unmarshalling fails
type UnmarshalError struct {
msg string
Expand Down Expand Up @@ -223,31 +241,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{errUnexpectedTxCall}
case Payment:
return &TransactionValidityError{"invalid payment"}
return &TransactionValidityError{errInvalidPayment}
case Future:
return &TransactionValidityError{"invalid transaction"}
return &TransactionValidityError{errInvalidTransaction}
case Stale:
return &TransactionValidityError{"outdated transaction"}
return &TransactionValidityError{errOutdatedTransaction}
case BadProof:
return &TransactionValidityError{"bad proof"}
return &TransactionValidityError{errBadProof}
case AncientBirthBlock:
return &TransactionValidityError{"ancient birth block"}
return &TransactionValidityError{errAncientBirthBlock}
case ExhaustsResources:
return &TransactionValidityError{"exhausts resources"}
return &TransactionValidityError{errExhaustsResources}
case InvalidCustom:
return &TransactionValidityError{fmt.Sprintf("unknown error: %d", val)}
return &TransactionValidityError{newUnknownError(val)}
case BadMandatory:
return &TransactionValidityError{"mandatory dispatch error"}
return &TransactionValidityError{errMandatoryDispatchError}
case MandatoryDispatch:
return &TransactionValidityError{"invalid mandatory dispatch"}
return &TransactionValidityError{errInvalidMandatoryDispatch}
case ValidityCannotLookup:
return &TransactionValidityError{"lookup failed"}
return &TransactionValidityError{errLookupFailed}
case NoUnsignedValidator:
return &TransactionValidityError{"validator not found"}
return &TransactionValidityError{errValidatorNotFound}
case UnknownCustom:
return &TransactionValidityError{fmt.Sprintf("unknown error: %d", val)}
return &TransactionValidityError{newUnknownError(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())
})
}
}