Proposer optimisation #6186

AdoAdoAdo · 2024-05-20T14:44:07Z

Reasoning behind the pull request

There is some degradation on the proposer during execution, which does not happen on the validators that vote the proposed block

Proposed changes

The process that takes most of this time is the management of generated smart contract results and their registration in the intermediate processors, that will be used afterwards in the case of reverts.
This will be refactored and optimized.

Testing procedure

run a testnet with multiple smart contract calls and monitor the processing time for proposers.
compare processing time between proposers and validators - these should be closer in time spent on processing the same block than it is now (some differences are still expected when there are many transactions, due to the initial filtering which happens on block proposal and not on block validation, but these should be limited to <100ms).

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…results.

gabi-vuls

Normal allin test: 1.7.11-f7efd0fc48 -> proposer-optimisation-e105bd9b53

--- Specific errors ---

block hash does not match 249
wrong nonce in block 215
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 1
Nr. of all WARNS: 252
Nr. of new ERRORS: 1
Nr. of new WARNS: 0
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

andreibancioiu · 2024-06-05T09:17:16Z

process/interface.go

@@ -170,7 +171,7 @@ type TransactionCoordinator interface {
 	VerifyCreatedBlockTransactions(hdr data.HeaderHandler, body *block.Body) error
 	GetCreatedInShardMiniBlocks() []*block.MiniBlock
 	VerifyCreatedMiniBlocks(hdr data.HeaderHandler, body *block.Body) error
-	AddIntermediateTransactions(mapSCRs map[block.Type][]data.TransactionHandler) error
+	AddIntermediateTransactions(mapSCRs map[block.Type][]data.TransactionHandler, key []byte) error


Maybe rename key to originalTransactionHash, to be more explicit?

Later edit: comment can be ignored, since key may have a different meaning.

andreibancioiu · 2024-06-05T09:34:17Z

process/block/postprocess/basePostProcess.go

-		bpp.mapProcessedResult[key] = append(bpp.mapProcessedResult[key], txHash)
+	pr, ok := bpp.mapProcessedResult[string(key)]
+	if !ok {
+		return


An extra confirmation - this is a valid case?

Normally should not exist, the intermediate results are added on a key that was previously registered, before starting the execution of the tx that generated the result.

Now the exception comes in case of scheduled txs, where the results are from previous block but are added on the next block and do not have any previously registered key (they don't need one as they don't need to revert).

In this case key comes as nil and it would not be found in the mapProcessedResult.

Wouldn't be better to check for key == nil in that case ?

andreibancioiu · 2024-06-05T09:41:11Z

process/block/postprocess/basePostProcess.go

@@ -28,6 +29,14 @@ type txInfo struct {
 	*txShardInfo
 }

+type processedResult struct {


Even is not exported, perhaps we can add some comments for each field, e.g. "the hash of the parent tx", "a lookup map with hashes of children txs" or something similar (since this part of the code is so important).

yes. a little bit more explicit names could be added.

renamed:
parent -> parentKey
children -> childrenKeys

the keys we are currently using are indeed the hashes, but could be anything.

andreibancioiu · 2024-06-05T10:55:59Z

process/block/postprocess/basePostProcess.go

+
+func (bpp *basePostProcessor) removeProcessedResultsAndLinks(key string) ([][]byte, bool) {
+	processedResults, ok := bpp.mapProcessedResult[key]
+	if !ok {


This case is for transactions without results?

right, also for blocks without miniblocks - valid case, miniblocks without transactions (hopefully we don't have this case but should be fine even if there are), or transactions without scrs - valid case.

sasurobert · 2024-06-06T09:04:46Z

process/block/postprocess/basePostProcess.go

@@ -28,6 +29,14 @@ type txInfo struct {
 	*txShardInfo
 }

+type processedResult struct {


yes. a little bit more explicit names could be added.

sasurobert · 2024-06-06T09:12:30Z

process/block/postprocess/intermediateResults.go

@@ -237,7 +238,7 @@ func (irp *intermediateResultsProcessor) VerifyInterMiniBlocks(body *block.Body)
 }

 // AddIntermediateTransactions adds smart contract results from smart contract processing for cross-shard calls
-func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.TransactionHandler) error {
+func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.TransactionHandler, key []byte) error {


key is too generic - add a name here - maybe miniBlockHash ?

this can be different things, such as a txhash, a scr hash or nil in case of scheduled txs results

sasurobert · 2024-06-06T10:47:43Z

process/block/postprocess/basePostProcess.go

+
+	bpp.mapProcessedResult[string(key)] = pr
+
+	if parentKey != nil {


len(parentKey) > 0 ?

…n-go into proposer-optimisation

AdoAdoAdo · 2024-06-06T12:58:09Z

process/block/postprocess/basePostProcess.go

@@ -28,6 +29,14 @@ type txInfo struct {
 	*txShardInfo
 }

+type processedResult struct {


renamed:
parent -> parentKey
children -> childrenKeys

the keys we are currently using are indeed the hashes, but could be anything.

AdoAdoAdo · 2024-06-06T13:14:16Z

process/block/postprocess/basePostProcess.go

+
+func (bpp *basePostProcessor) removeProcessedResultsAndLinks(key string) ([][]byte, bool) {
+	processedResults, ok := bpp.mapProcessedResult[key]
+	if !ok {


right, also for blocks without miniblocks - valid case, miniblocks without transactions (hopefully we don't have this case but should be fine even if there are), or transactions without scrs - valid case.

AdoAdoAdo · 2024-06-14T11:57:43Z

process/block/postprocess/basePostProcess.go

+
+	bpp.mapProcessedResult[string(key)] = pr
+
+	if parentKey != nil {


AdoAdoAdo · 2024-06-14T12:03:34Z

process/block/postprocess/basePostProcess.go

-		bpp.mapProcessedResult[key] = append(bpp.mapProcessedResult[key], txHash)
+	pr, ok := bpp.mapProcessedResult[string(key)]
+	if !ok {
+		return


Normally should not exist, the intermediate results are added on a key that was previously registered, before starting the execution of the tx that generated the result.

Now the exception comes in case of scheduled txs, where the results are from previous block but are added on the next block and do not have any previously registered key (they don't need one as they don't need to revert).

In this case key comes as nil and it would not be found in the mapProcessedResult.

AdoAdoAdo · 2024-06-14T12:11:34Z

process/block/postprocess/intermediateResults.go

@@ -237,7 +238,7 @@ func (irp *intermediateResultsProcessor) VerifyInterMiniBlocks(body *block.Body)
 }

 // AddIntermediateTransactions adds smart contract results from smart contract processing for cross-shard calls
-func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.TransactionHandler) error {
+func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.TransactionHandler, key []byte) error {


this can be different things, such as a txhash, a scr hash or nil in case of scheduled txs results

AdoAdoAdo · 2024-06-14T12:21:34Z

process/transaction/shardProcess.go

@@ -963,7 +964,7 @@ func (txProc *txProcessor) executeFailedRelayedUserTx(
 		return err
 	}

-	err = txProc.scrForwarder.AddIntermediateTransactions([]data.TransactionHandler{scrForRelayer})
+	err = txProc.scrForwarder.AddIntermediateTransactions([]data.TransactionHandler{scrForRelayer}, originalTxHash)


this should be verified, if the originalTxHash or the userTxHash should be used.

originalTxHash has to be used. as in case of revert, the used key for this is the originalTxHash.

AdoAdoAdo · 2024-06-14T12:22:23Z

process/transaction/shardProcess.go

@@ -985,7 +986,7 @@ func (txProc *txProcessor) executeFailedRelayedUserTx(
 		}

 		if txProc.enableEpochsHandler.IsFlagEnabled(common.AddFailedRelayedTxToInvalidMBsFlag) {
-			err = txProc.badTxForwarder.AddIntermediateTransactions([]data.TransactionHandler{originalTx})
+			err = txProc.badTxForwarder.AddIntermediateTransactions([]data.TransactionHandler{originalTx}, originalTxHash)


should be checked here as well if userTxHash should be used.

originalTxHash has to be used. as in case of revert, the used key for this is the originalTxHash.

AdoAdoAdo · 2024-06-14T12:25:58Z

process/transaction/shardProcess.go

@@ -890,7 +891,7 @@ func (txProc *txProcessor) processUserTx(
 		return returnCode, nil
 	}

-	err = txProc.scrForwarder.AddIntermediateTransactions([]data.TransactionHandler{scrFromTx})
+	err = txProc.scrForwarder.AddIntermediateTransactions([]data.TransactionHandler{scrFromTx}, txHash)


txhash is for relay transaction, should be checked if here we should use the user tx hash instead.

originalTxHash has to be used. as in case of revert, the used key for this is the originalTxHash.

This could be check with an integration test as well.

gabi-vuls

Normal allin test: v1.7.13-dev-config-aebb55e8e0 -> proposer-optimisation-6042eaaf4e

--- Specific errors ---

block hash does not match 327
wrong nonce in block 235
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 247
Nr. of new ERRORS: 0
Nr. of new WARNS: 0
Nr. of PANICS: 0

/------/

--- ERRORS ---

/------/

--- WARNINGS ---

/------/

AdoAdoAdo added 2 commits May 16, 2024 13:16

add key argument on AddIntermediateTransactions

69c2cd2

Link keys between headers, miniblocks and transactions for processed …

e105bd9

…results.

gabi-vuls previously approved these changes May 24, 2024

View reviewed changes

AdoAdoAdo marked this pull request as ready for review June 4, 2024 07:50

Merge branch 'master' into proposer-optimisation

5b78b02

BeniaminDrasovean self-requested a review June 5, 2024 09:03

andreibancioiu self-requested a review June 5, 2024 09:03

BeniaminDrasovean previously approved these changes Jun 5, 2024

View reviewed changes

andreibancioiu assigned AdoAdoAdo Jun 5, 2024

andreibancioiu reviewed Jun 5, 2024

View reviewed changes

sasurobert self-requested a review June 6, 2024 08:55

sasurobert reviewed Jun 6, 2024

View reviewed changes

AdoAdoAdo added 2 commits June 14, 2024 15:32

fixes after review plus some tests

ff5ce51

Merge branch 'proposer-optimisation' of github.com:multiversx/mx-chai…

a75c3df

…n-go into proposer-optimisation

AdoAdoAdo dismissed stale reviews from BeniaminDrasovean and gabi-vuls via a75c3df June 14, 2024 12:33

AdoAdoAdo commented Jun 14, 2024

View reviewed changes

remove profiling

599ea76

AdoAdoAdo changed the base branch from master to rc/v1.7.next1 June 14, 2024 12:40

AdoAdoAdo and others added 6 commits June 14, 2024 15:40

Merge branch 'rc/v1.7.next1' into proposer-optimisation

0505728

update gosum

75efb5e

Merge branch 'rc/v1.7.next1' into proposer-optimisation

edcc3a5

Merge branch 'rc/v1.7.next1' into proposer-optimisation

6042eaa

update gosum

b19c5bf

Merge branch 'rc/v1.7.next1' into proposer-optimisation

64253b0

sasurobert approved these changes Jul 4, 2024

View reviewed changes

andreibancioiu approved these changes Jul 4, 2024

View reviewed changes

BeniaminDrasovean approved these changes Jul 4, 2024

View reviewed changes

gabi-vuls approved these changes Jul 4, 2024

View reviewed changes

gabi-vuls reviewed Jul 4, 2024

View reviewed changes

AdoAdoAdo merged commit 313b62e into rc/v1.7.next1 Jul 4, 2024
8 checks passed

AdoAdoAdo deleted the proposer-optimisation branch July 4, 2024 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposer optimisation #6186

Proposer optimisation #6186

AdoAdoAdo commented May 20, 2024 •

edited

Loading

gabi-vuls left a comment •

edited

Loading

andreibancioiu Jun 5, 2024 •

edited

Loading

andreibancioiu Jun 5, 2024

AdoAdoAdo Jun 14, 2024

sasurobert Jul 2, 2024

andreibancioiu Jun 5, 2024 •

edited

Loading

sasurobert Jun 6, 2024

AdoAdoAdo Jun 6, 2024

andreibancioiu Jun 5, 2024

AdoAdoAdo Jun 6, 2024

sasurobert Jun 6, 2024

sasurobert Jun 6, 2024

AdoAdoAdo Jun 14, 2024

sasurobert Jun 6, 2024

AdoAdoAdo Jun 14, 2024

AdoAdoAdo Jun 6, 2024

AdoAdoAdo Jun 6, 2024

AdoAdoAdo Jun 14, 2024

AdoAdoAdo Jun 14, 2024

AdoAdoAdo Jun 14, 2024

AdoAdoAdo Jun 14, 2024

sasurobert Jul 2, 2024

AdoAdoAdo Jun 14, 2024

sasurobert Jul 2, 2024

AdoAdoAdo Jun 14, 2024

sasurobert Jul 2, 2024

sasurobert Jul 2, 2024

gabi-vuls left a comment


		bpp.mapProcessedResult[string(key)] = pr

		if parentKey != nil {

Proposer optimisation #6186

Proposer optimisation #6186

Conversation

AdoAdoAdo commented May 20, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

gabi-vuls left a comment • edited Loading

Choose a reason for hiding this comment

andreibancioiu Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

AdoAdoAdo commented May 20, 2024 •

edited

Loading

gabi-vuls left a comment •

edited

Loading

andreibancioiu Jun 5, 2024 •

edited

Loading

andreibancioiu Jun 5, 2024 •

edited

Loading