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

fix(dot/core): check transaction Validity.Propagate field to determine whether to propagate tx #1643

Merged
merged 19 commits into from
Jun 23, 2021
Merged

fix(dot/core): check transaction Validity.Propagate field to determine whether to propagate tx #1643

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
04b1aaf
check validate.Propagate field
edwardmack Jun 15, 2021
1094eee
Merge branch 'development' into ed/checkValidityPropagate
edwardmack Jun 16, 2021
9417c83
add functionality to remove non-propagate txs
edwardmack Jun 16, 2021
39071d8
fix mocks
edwardmack Jun 16, 2021
38850fe
Merge branch 'development' into ed/checkValidityPropagate
edwardmack Jun 16, 2021
f2b8582
chore: adding verbosity to golangci
EclesioMeloJunior Jun 16, 2021
aee53d3
chore: change the out-format argument golangci
EclesioMeloJunior Jun 16, 2021
49d5b71
using print-issued-lines arg
EclesioMeloJunior Jun 16, 2021
bd77c71
ínstalling golangci instead use github actions
EclesioMeloJunior Jun 16, 2021
5d84423
lint
edwardmack Jun 17, 2021
04a2750
Merge branch 'development' into ed/checkValidityPropagate
edwardmack Jun 17, 2021
7f32dae
Merge branch 'development' into ed/checkValidityPropagate
edwardmack Jun 18, 2021
9638b61
clean-up logic checks
edwardmack Jun 18, 2021
1f31c57
Merge branch 'development' into ed/checkValidityPropagate
edwardmack Jun 19, 2021
7dd754a
run make mock
edwardmack Jun 21, 2021
54ac571
update comments, fix mock output
edwardmack Jun 21, 2021
e4bb319
add test for bogus extrinsic
edwardmack Jun 22, 2021
43aeeef
fix spelling
edwardmack Jun 22, 2021
1d74a8b
change logging level
edwardmack Jun 22, 2021
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: 3 additions & 3 deletions .github/workflows/checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ jobs:
run: diff -u <(echo -n) <(gofmt -d -s .)

- name: golangci-lint
uses: golangci/golangci-lint-action@v2
with:
version: 'latest'
run: |
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.41.0
$(go env GOPATH)/bin/golangci-lint run

vet-check:
runs-on: ubuntu-latest
Expand Down
33 changes: 28 additions & 5 deletions dot/core/messages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,31 @@
package core

import (
"reflect"

"github.com/ChainSafe/gossamer/dot/network"
"github.com/ChainSafe/gossamer/dot/types"
"github.com/ChainSafe/gossamer/lib/transaction"
)

// HandleTransactionMessage validates each transaction in the message and
// adds valid transactions to the transaction queue of the BABE session
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have a description that the bool return means there is extrinsic to propagate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point... Updated.

func (s *Service) HandleTransactionMessage(msg *network.TransactionMessage) error {
func (s *Service) HandleTransactionMessage(msg *network.TransactionMessage) (bool, error) {
logger.Debug("received TransactionMessage")
if !s.isBlockProducer {
return nil
return false, nil
}
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 these lines, we should be validating + propagating tx messages even if we aren't block producers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.


// get transactions from message extrinsics
txs := msg.Extrinsics

var toRemove []types.Extrinsic
for _, tx := range txs {
// validate each transaction
externalExt := types.Extrinsic(append([]byte{byte(types.TxnExternal)}, tx...))
val, err := s.rt.ValidateTransaction(externalExt)
if err != nil {
logger.Error("failed to validate transaction", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this to debug since it's not an 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.

good point, updated.

return err
return false, err
Copy link
Contributor

Choose a reason for hiding this comment

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

don't return here but continue, and mark this tx "do not propagate" instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated.

}

// create new valid transaction
Expand All @@ -48,7 +50,28 @@ func (s *Service) HandleTransactionMessage(msg *network.TransactionMessage) erro
// push to the transaction queue of BABE session
hash := s.transactionState.AddToPool(vtx)
logger.Trace("Added transaction to queue", "hash", hash)

// find tx(s) that should not propagate
if !val.Propagate {
toRemove = append(toRemove, tx)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be more straightforward to have a list of extrinsics to propagate instead of toRemove, and add valid txs with Propagate == true to it, that way you don't need to use findAndDelete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, more straightforward and readable, updated.

}

// remove tx(s) that should not propagate
for _, v := range toRemove {
msg.Extrinsics = findAndDelete(msg.Extrinsics, v)
}

return nil
return len(msg.Extrinsics) > 0, nil
}

func findAndDelete(s []types.Extrinsic, item types.Extrinsic) []types.Extrinsic {
index := 0
for _, i := range s {
if !reflect.DeepEqual(i, item) {
s[index] = i
index++
}
}
return s[:index]
}
3 changes: 2 additions & 1 deletion dot/core/messages_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,9 @@ func TestService_HandleTransactionMessage(t *testing.T) {
extBytes := createExtrinsics(t, s.rt, genHash, 0)

msg := &network.TransactionMessage{Extrinsics: []types.Extrinsic{extBytes}}
err = s.HandleTransactionMessage(msg)
b, err := s.HandleTransactionMessage(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add a test case for invalid extrinsics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

require.NoError(t, err)
require.True(t, b)

pending := s.transactionState.(*state.TransactionState).Pending()
require.NotEqual(t, 0, len(pending))
Expand Down
17 changes: 12 additions & 5 deletions dot/network/mock_transaction_handler.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dot/network/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,5 @@ type Syncer interface {

// TransactionHandler is the interface used by the transactions sub-protocol
type TransactionHandler interface {
HandleTransactionMessage(*TransactionMessage) error
HandleTransactionMessage(*TransactionMessage) (bool, error)
}
2 changes: 1 addition & 1 deletion dot/network/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,5 @@ func (s *Service) handleTransactionMessage(_ peer.ID, msg NotificationsMessage)
return false, errors.New("invalid transaction type")
}

return true, s.transactionHandler.HandleTransactionMessage(txMsg)
return s.transactionHandler.HandleTransactionMessage(txMsg)
}
8 changes: 8 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ
github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
github.com/hashicorp/golang-lru v0.5.5-0.20210104140557-80c98217689d h1:dg1dEPuWpEqDnvIw251EVy4zlP8gWbsGj4BsUKCRpYs=
github.com/hashicorp/golang-lru v0.5.5-0.20210104140557-80c98217689d/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
github.com/holiman/bloomfilter/v2 v2.0.3 h1:73e0e/V0tCydx14a0SCYS/EWCxgwLZ18CZcZKVu0fao=
github.com/holiman/bloomfilter/v2 v2.0.3/go.mod h1:zpoh+gs7qcpqrHr3dB55AMiJwo0iURXE7ZOP9L9hSkA=
Expand All @@ -187,6 +188,7 @@ github.com/huin/goupnp v1.0.0/go.mod h1:n9v9KO1tAxYH82qOn+UTIFQDmx5n1Zxd/ClZDMX7
github.com/huin/goupnp v1.0.1-0.20200620063722-49508fba0031 h1:HarGZ5h9HD9LgEg1yRVMXyfiw4wlXiLiYM2oMjeA/SE=
github.com/huin/goupnp v1.0.1-0.20200620063722-49508fba0031/go.mod h1:nNs7wvRfN1eKaMknBydLNQU6146XQim8t4h+q90biWo=
github.com/huin/goutil v0.0.0-20170803182201-1ca381bf3150/go.mod h1:PpLOETDnJ0o3iZrZfqZzyLl6l7F3c6L1oWn7OICBi6o=
github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
github.com/ipfs/go-cid v0.0.1/go.mod h1:GHWU/WuQdMPmIosc4Yn1bcCT7dSeX4lBafM7iqUPQvM=
github.com/ipfs/go-cid v0.0.2/go.mod h1:GHWU/WuQdMPmIosc4Yn1bcCT7dSeX4lBafM7iqUPQvM=
Expand Down Expand Up @@ -490,7 +492,9 @@ github.com/minio/sha256-simd v0.1.0/go.mod h1:2FMWW+8GMoPweT6+pI63m9YE3Lmw4J71hV
github.com/minio/sha256-simd v0.1.1-0.20190913151208-6de447530771/go.mod h1:B5e1o+1/KgNmWrSQK08Y6Z1Vb5pwIktudl0J58iy0KM=
github.com/minio/sha256-simd v0.1.1 h1:5QHSlgo3nt5yKOJrC7W8w7X+NFl8cMPZm96iu8kKUJU=
github.com/minio/sha256-simd v0.1.1/go.mod h1:B5e1o+1/KgNmWrSQK08Y6Z1Vb5pwIktudl0J58iy0KM=
github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y=
github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0=
github.com/mitchellh/mapstructure v1.1.2 h1:fmNYVwqnSfB9mZU6OS2O6GsXM+wcskZDuKQzvN1EDeE=
github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
github.com/mr-tron/base58 v1.1.0/go.mod h1:xcD2VGqlgYjBdcBLw+TuYLr8afG+Hj8g2eTVqeSzSU8=
github.com/mr-tron/base58 v1.1.1/go.mod h1:xcD2VGqlgYjBdcBLw+TuYLr8afG+Hj8g2eTVqeSzSU8=
Expand Down Expand Up @@ -576,6 +580,7 @@ github.com/opentracing/opentracing-go v1.0.2/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFSt
github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o=
github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs=
github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc=
github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc=
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
github.com/perlin-network/life v0.0.0-20191203030451-05c0e0f7eaea h1:okKoivlkNRRLqXraEtatHfEhW+D71QTwkaj+4n4M2Xc=
github.com/perlin-network/life v0.0.0-20191203030451-05c0e0f7eaea/go.mod h1:3KEU5Dm8MAYWZqity880wOFJ9PhQjyKVZGwAEfc5Q4E=
Expand Down Expand Up @@ -604,9 +609,12 @@ github.com/spacemonkeygo/spacelog v0.0.0-20180420211403-2296661a0572/go.mod h1:w
github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0bLI=
github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
github.com/spf13/afero v1.1.2 h1:m8/z1t7/fwjysjQRYbP0RD+bUIF/8tJwPdEZsI83ACI=
github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=
github.com/spf13/cast v1.3.0 h1:oget//CVOEoFewqQxwr0Ej5yjygnqGkvggSE/gB35Q8=
github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE=
github.com/spf13/cobra v0.0.5/go.mod h1:3K3wKZymM7VvHMDS9+Akkh4K60UwM26emMESw8tLCHU=
github.com/spf13/jwalterweatherman v1.0.0 h1:XHEdyB+EcvlqZamSM4ZOMGlc93t6AcsBEu9Gc1vn7yk=
github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo=
github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s=
Expand Down
2 changes: 1 addition & 1 deletion tests/polkadotjs_test/start_polkadotjs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestStartGossamerAndPolkadotAPI(t *testing.T) {
command := "npx mocha ./test"
parts := strings.Fields(command)
data, err := exec.Command(parts[0], parts[1:]...).Output()
require.NoError(t, err, fmt.Sprintf("%s", data))
require.NoError(t, err, data)

//uncomment this to see log results from javascript tests
//fmt.Printf("%s\n", data)
Expand Down