[Exec] Update sequence number independent of transaction invocation result #602

ramtinms · 2021-04-08T22:01:17Z

This PR

changes the behaviour of sequence number update as follows
- FVM should not update the seq number if seq number verification fails (invalid signatures, invalid seq number, non-existence proposal account, ...) .
- FVM should do update the seq number no matter the results of tx invocation and storage limiting.
- Execution and Verification node should always merge back the tx View
fixes error handling of meta transactions. (the txErrors were ignored previously)

Most of the heavy lifting has been done in previous PRs, this PR adds few tests and fix exec node's computer and chunk verifier and adds some extra unit test.

What does this mean?

Starting next spork, when running a transaction, the proposer key sequence number would always be incremented unless any of the following conditions occur:

invalid transaction (transaction rejected by collection nodes)
any of the transaction signatures are not valid
the transaction is missing the signature(s) by the proposer
the proposal sequence number is invalid (not match with the one stored on the chain)
In other words, the new behaviour increments the sequence number even if the transaction fails at runtime or any other condition that is not mentioned above. The new behaviour protects the network against some cases of spamming attacks and protects users from double transaction submission attacks.

…eq-number-updates-on-failed-tx

ramtinms · 2021-04-13T21:17:12Z

engine/execution/computation/programs_test.go

-	prepareTx(t, tx6, account, privKey, 4, chain)
+	prepareTx(t, tx6, account, privKey, 5, chain)


the previous call in the test supposed to fail due to runtime error (failed in updating contract), previously we didn't increment the sequence number but with the new behaviour it needs to increment.

ramtinms · 2021-04-13T21:17:22Z

engine/execution/execution_test.go

+	// we don't set the proper sequence number of this one
+	_, col2, block2, proposal2, _ := makePanicBlock(t, conID, colID, chain, uint64(0), block1.Header, genesis)


ramtinms · 2021-04-13T21:21:18Z

fvm/fvm_test.go

+			}),
+	)
+
+	t.Run("Transaction invocation fails but sequence number is incremented", newVMTest().withBootstrapProcedureOptions(


We also have added this test before

ramtinms · 2021-04-13T21:21:45Z

fvm/transactionFeeDeductor.go

@@ -25,7 +26,12 @@ func (d *TransactionFeeDeductor) Process(
 		defer span.Finish()
 	}

-	return d.deductFees(vm, ctx, proc.Transaction, sth, programs)
+	txErr, fatalErr := d.deductFees(vm, ctx, proc.Transaction, sth, programs)
+	// TODO handle deduct fee failures, for now just return as error


we need to address this case on fee deduction PR

ramtinms · 2021-04-13T22:08:34Z

module/chunks/chunkVerifier_test.go

-		_ = led.Set("00", "", "", []byte{'F'})
+		_ = led.Set("05", "", "", []byte{'B'})


this needs to change the state commitment as the sequence number is applied.

ramtinms · 2021-04-13T22:09:53Z

fvm/transactionSequenceNum_test.go

+
+		tx := flow.TransactionBody{}
+		tx.SetProposalKey(address, 0, 0)
+		signEnvelope(&tx, address, *privKey)


signatures are not needed for this test but I added them here, so in the future when we move proposal signature verification to this component, the test will check it.

…com:onflow/flow-go into ramtin/4107-seq-number-updates-on-failed-tx

engine/execution/computation/computer/computer.go

fvm/env.go

fvm/bootstrap.go

…cation result (#602)" This reverts commit ad6c469.

* Revert "[Exec] Update sequence number independent of transaction invocation result (#602)" This reverts commit ad6c469. * Keep fvm changes

ramtinms added 4 commits April 8, 2021 14:07

don't handle tx invocation failures at computer

a945690

fix tests

e0d63b4

fix chunk verifier

0e020a7

fix test to reflect the changes

5fb6d74

ramtinms changed the title ~~[WIP] update seq number even if tx invocation fails.~~ [WIP] update seq number even if tx invocation fails Apr 9, 2021

ramtinms and others added 3 commits April 8, 2021 22:52

Merge branch 'master' into ramtin/4107-seq-number-updates-on-failed-tx

3093b94

Merge branch 'master' of github.com:onflow/flow-go into ramtin/4107-s…

51c0f18

…eq-number-updates-on-failed-tx

handle invoke meta tx errors

3266887

ramtinms commented Apr 13, 2021

View reviewed changes

handel bootstrapping tx errors

5324ee8

ramtinms commented Apr 13, 2021

View reviewed changes

add sequence number check tests

3b225b7

ramtinms commented Apr 13, 2021

View reviewed changes

ramtinms changed the title ~~[WIP] update seq number even if tx invocation fails~~ Update seq number even if tx invocation fails Apr 13, 2021

ramtinms and others added 6 commits April 13, 2021 15:12

fix linter

30d4296

no need to signature

b101e57

fix linter

c7c9d15

Merge branch 'master' into ramtin/4107-seq-number-updates-on-failed-tx

867b7e1

hot fix

62eb330

Merge branch 'ramtin/4107-seq-number-updates-on-failed-tx' of github.…

3842091

…com:onflow/flow-go into ramtin/4107-seq-number-updates-on-failed-tx

ramtinms marked this pull request as ready for review April 13, 2021 23:22

ramtinms requested review from m4ksio and psiemens as code owners April 13, 2021 23:22

ramtinms requested review from janezpodhostnik, jordanschalm and zhangchiqing April 13, 2021 23:22

ramtinms changed the title ~~Update seq number even if tx invocation fails~~ [Exec] Update sequence number independent of transaction invocation result Apr 13, 2021

m4ksio approved these changes Apr 13, 2021

View reviewed changes

engine/execution/computation/computer/computer.go Outdated Show resolved Hide resolved

fvm/env.go Outdated Show resolved Hide resolved

ramtinms and others added 2 commits April 13, 2021 17:26

Merge branch 'master' into ramtin/4107-seq-number-updates-on-failed-tx

6917ef9

apply PR's comments

bd09459

janezpodhostnik approved these changes Apr 14, 2021

View reviewed changes

fvm/bootstrap.go Outdated Show resolved Hide resolved

ramtinms and others added 3 commits April 14, 2021 13:08

apply pr's comments

be2e6e8

Merge branch 'master' into ramtin/4107-seq-number-updates-on-failed-tx

bb824d8

Merge branch 'master' into ramtin/4107-seq-number-updates-on-failed-tx

0ab3de9

ramtinms merged commit ad6c469 into master Apr 15, 2021

ramtinms deleted the ramtin/4107-seq-number-updates-on-failed-tx branch April 15, 2021 18:37

Kay-Zee added a commit that referenced this pull request Apr 27, 2021

Revert "[Exec] Update sequence number independent of transaction invo…

07f7612

…cation result (#602)" This reverts commit ad6c469.

Kay-Zee added a commit that referenced this pull request Apr 27, 2021

Kan/revert sequence number bug fix (#663)

44d2290

* Revert "[Exec] Update sequence number independent of transaction invocation result (#602)" This reverts commit ad6c469. * Keep fvm changes

kaliweh-circle mentioned this pull request Jul 29, 2021

Increment the proposer key sequence for failed transactions onflow/flow-emulator#67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Exec] Update sequence number independent of transaction invocation result #602

[Exec] Update sequence number independent of transaction invocation result #602

ramtinms commented Apr 8, 2021 •

edited

ramtinms Apr 13, 2021

ramtinms Apr 13, 2021

ramtinms Apr 13, 2021

ramtinms Apr 13, 2021

ramtinms Apr 13, 2021

ramtinms Apr 13, 2021

		prepareTx(t, tx6, account, privKey, 4, chain)
		prepareTx(t, tx6, account, privKey, 5, chain)

		// we don't set the proper sequence number of this one
		_, col2, block2, proposal2, _ := makePanicBlock(t, conID, colID, chain, uint64(0), block1.Header, genesis)

		_ = led.Set("00", "", "", []byte{'F'})
		_ = led.Set("05", "", "", []byte{'B'})

[Exec] Update sequence number independent of transaction invocation result #602

[Exec] Update sequence number independent of transaction invocation result #602

Conversation

ramtinms commented Apr 8, 2021 • edited

What does this mean?

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms Apr 13, 2021

Choose a reason for hiding this comment

ramtinms commented Apr 8, 2021 •

edited