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

Chain id and version in transaction structure #2040

Merged
merged 21 commits into from
Jul 3, 2020

Conversation

miiu96
Copy link
Contributor

@miiu96 miiu96 commented Jun 29, 2020

Added chain ID and version in transaction structure. Now all transaction that are generated (from wallet, erdpy, tx-gen etc must contains correct chain ID and version in order to be accepted by the observers). Also the transaction must be signed with the chain ID and version.

# Conflicts:
#	cmd/node/factory/structs.go
#	epochStart/bootstrap/factory/epochStartInterceptorsContainerFactory.go
#	integrationTests/multiShard/transaction/interceptedResolvedBulkTx/interceptedResolvedBulkTx_test.go
#	integrationTests/singleShard/transaction/interceptedBulkTx/interceptedBulkTx_test.go
#	integrationTests/testProcessorNode.go
#	node/node.go
#	node/nodeTesting.go
#	node/nodeTesting_test.go
#	process/factory/interceptorscontainer/args.go
#	process/factory/interceptorscontainer/metaInterceptorsContainerFactory.go
#	process/factory/interceptorscontainer/metaInterceptorsContainerFactory_test.go
#	process/factory/interceptorscontainer/shardInterceptorsContainerFactory.go
#	process/factory/interceptorscontainer/shardInterceptorsContainerFactory_test.go
#	process/interceptors/factory/argInterceptedDataFactory.go
#	process/interceptors/factory/interceptedMetaHeaderDataFactory_test.go
#	process/interceptors/factory/interceptedTxDataFactory.go
#	process/transaction/interceptedTransaction.go
#	process/transaction/interceptedTransaction_test.go
#	update/factory/fullSyncInterceptors.go
sasurobert
sasurobert previously approved these changes Jun 29, 2020
@raduchis raduchis self-requested a review June 29, 2020 15:38
raduchis
raduchis previously approved these changes Jun 29, 2020
10,
100000,
[]byte("data@~`!@#$^&*()_=[]{};'<>?,./|<>><!!!!!"),
[]byte("erd-test-data"),
Copy link
Contributor

Choose a reason for hiding this comment

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

you discarded the special characters. Does they still work?

Copy link
Contributor

Choose a reason for hiding this comment

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

they work. I just generated something different for Mihai to test and forgot to add back that data field. Will be updated

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 again special characters

@@ -216,6 +222,9 @@ func (inTx *InterceptedTransaction) processFields(txBuff []byte) error {

// integrity checks for not nil fields and negative value
func (inTx *InterceptedTransaction) integrity(tx *transaction.Transaction) error {
if tx.ChainID == nil || !bytes.Equal(tx.ChainID, inTx.chainID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do this directly:

if !bytes.Equal(tx.ChainID, inTx.chainID) {

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.

# Conflicts:
#	integrationTests/multiShard/block/executingMiniblocks/executingMiniblocks_test.go
@miiu96 miiu96 dismissed stale reviews from raduchis and sasurobert via f2871eb June 30, 2020 06:33
@miiu96 miiu96 changed the title Chain id in transaction structure Chain id and version in transaction structure Jun 30, 2020
@miiu96 miiu96 self-assigned this Jun 30, 2020
@@ -26,7 +26,7 @@ type Facade struct {
GenerateTransactionHandler func(sender string, receiver string, value *big.Int, code string) (*transaction.Transaction, error)
GetTransactionHandler func(hash string) (*transaction.ApiTransactionResult, error)
CreateTransactionHandler func(nonce uint64, value string, receiverHex string, senderHex string, gasPrice uint64,
gasLimit uint64, data string, signatureHex string, chainID string) (*transaction.Transaction, []byte, error)
gasLimit uint64, data string, signatureHex string, chainID string, version uint32) (*transaction.Transaction, []byte, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

do not forget to expose the minimum transaction version on the network config route (in order to be used by the frontend). Also, create PR to update the proxy/txgen repos. Frontend should be upgraded too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests all over the PR where the new data field are verified.

Copy link
Contributor Author

@miiu96 miiu96 Jul 1, 2020

Choose a reason for hiding this comment

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

Already exposed minimum transaction version in api route /network/config. And PR for proxy and tx-gen will be opened soon (work in progress).

Copy link
Contributor Author

@miiu96 miiu96 Jul 1, 2020

Choose a reason for hiding this comment

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

more unit tests added

Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

One more thing that was not obvious until now.

@@ -76,6 +76,12 @@ func NewShardInterceptorsContainerFactory(
if check.IfNil(args.EpochStartTrigger) {
return nil, process.ErrNilEpochStartTrigger
}
if len(args.ChainID) == 0 {
return nil, process.ErrInvalidChainID
Copy link
Contributor

Choose a reason for hiding this comment

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

Please treat those 2 errors in multidata and single data interceptor and blacklist the fromconnected peer and message peer originator

Copy link
Contributor

Choose a reason for hiding this comment

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

In multidatainterceptor.go, L186 and in singledatainterceptor.go L95 please add:

		isWrongVersion := err == process.ErrInvalidTransactionVersion || err == process.ErrInvalidChainID
		if isWrongVersion{
			//this situation is so severe that we need to black list de peers
			reason := "wrong version of received intercepted data, topic " + mdi.topic + ", error " + err.Error()
			mdi.antifloodHandler.BlacklistPeer(originator, reason, core.InvalidMessageBlacklistDuration)
			mdi.antifloodHandler.BlacklistPeer(fromConnectedPeer, reason, core.InvalidMessageBlacklistDuration)
		} 

please add tests for these condition. Thanks

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.

iulianpascalau
iulianpascalau previously approved these changes Jul 2, 2020
processReceivedMessageMultiDataInvalidVersion(t, process.ErrInvalidChainID)
}

func processReceivedMessageMultiDataInvalidVersion(t *testing.T, expectedErr error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

BlacklistPeerCalled: func(peer core.PeerID, reason string, duration time.Duration) {
switch string(peer) {
case string(originator):
isOriginatorBlackListed = true
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

sasurobert
sasurobert previously approved these changes Jul 2, 2020
@miiu96 miiu96 dismissed stale reviews from sasurobert and iulianpascalau via 4730fe7 July 2, 2020 15:05
Copy link
Contributor

@raduchis raduchis left a comment

Choose a reason for hiding this comment

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

we might think also about currentVersion sometime

Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit 4cce44a into development Jul 3, 2020
@LucianMincu LucianMincu deleted the add-chain-id-in-transaction branch July 3, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants