Feat/pubkeyconverter_refactor #4716

schimih · 2022-11-18T16:37:56Z

Reasoning behind the pull request

The original implementation of the pidPubkeyConverter.Encode method did not propagate the error to its caller. Instead, it logged the error and returned an empty string. This behavior can be problematic as it might hide potential issues and security threats. Furthermore, the logger's presence in the PubKeyConverter constructor became unnecessary, therefore the hrp was given as input, especially considering our vision for sovereign shards where each shard could have its unique human-readable part (hrp).

Proposed changes

refactor the pidPubkeyConverter.Encode method by returning both a string and an error.
remove the log statement, which was previously used to log the error;
return the error so that the caller should decide what to do with the error (handle or log);
provide hrp as input parameter for the PubKeyConverter constructor;
treat the error everywhere the NewPubKeyConverter constructor is used;
Legacy refactor:

Testing procedure

standard system test
run a blockchain with half of the nodes using the previous node version and the other half with this branch

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?

…h32_addresses_format

Merge rcv1.5.0 2022.12.08 to pubkeyconvert-refactor

…h32_addresses_format

codecov-commenter · 2022-12-09T09:45:37Z

Codecov Report

❗ No coverage uploaded for pull request base (rc/v1.6.0@89b54e7). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 2182749 differs from pull request most recent head 58c40b0. Consider uploading reports for the commit 58c40b0 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff              @@
##             rc/v1.6.0    #4716   +/-   ##
============================================
  Coverage             ?   70.89%           
============================================
  Files                ?      675           
  Lines                ?    87614           
  Branches             ?        0           
============================================
  Hits                 ?    62118           
  Misses               ?    20827           
  Partials             ?     4669

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…v_23feb merge rc/v1.6.0 in pubkeyConverter

Merge branch 'rc/v1.6.0' into feat/pubkey_conv

…3.10 # Conflicts: # cmd/keygenerator/converter/errors.go # cmd/keygenerator/converter/pidPubkeyConverter.go # cmd/keygenerator/converter/pidPubkeyConverter_test.go # cmd/keygenerator/main.go # common/enablers/enableEpochsHandler.go # common/factory/pubkeyConverterFactory.go # config/tomlConfig_test.go # examples/construction_test.go # factory/addressDecoder_test.go # factory/crypto/cryptoComponents.go # genesis/checking/nodesSetupChecker.go # genesis/checking/nodesSetupChecker_test.go # genesis/parsing/accountsParser.go # genesis/parsing/accountsParser_test.go # go.mod # go.sum # integrationTests/consensus/consensus_test.go # integrationTests/frontend/wallet/dataField_test.go # integrationTests/multiShard/txScenarios/moveBalance_test.go # integrationTests/testConsensusNode.go # integrationTests/vm/txsFee/scCalls_test.go # node/external/nodeApiResolver.go # node/external/transactionAPI/apiTransactionProcessor.go # node/external/transactionAPI/apiTransactionProcessor_test.go # node/external/transactionAPI/apiTransactionResults.go # node/external/transactionAPI/gasUsedAndFeeProcessor_test.go # node/external/transactionAPI/unmarshaller.go # node/external/transactionAPI/unmarshaller_test.go # node/trieIterators/delegatedListProcessor_test.go # node/trieIterators/directStakedListProcessor.go # node/trieIterators/directStakedListProcessor_test.go # node/trieIterators/factory/delegatedListHandlerFactory_test.go # node/trieIterators/factory/directStakedListHandlerFactory_test.go # node/trieIterators/factory/stakeValuesHandlerFactory_test.go # node/trieIterators/stakeValuesProcessor_test.go # outport/factory/outportFactory_test.go # outport/process/alteredaccounts/alteredAccountsProvider.go # outport/process/transactionsfee/transactionsFeeProcessor.go # outport/process/transactionsfee/transactionsFeeProcessor_test.go # process/block/postprocess/intermediateResults_test.go # process/block/preprocess/transactions_test.go # process/dataValidators/txValidator.go # process/factory/metachain/intermediateProcessorsContainerFactory_test.go # process/peer/validatorsProvider_test.go # process/rewardTransaction/interceptedRewardTransaction_test.go # process/smartContract/process.go # process/txsimulator/txSimulator.go # process/unsigned/interceptedUnsignedTransaction_test.go # testscommon/addresses.go # testscommon/integrationtests/stringers.go # testscommon/integrationtests/stringers_test.go # update/genesis/export_test.go # vm/systemSmartContracts/esdt_test.go

…conv-2023.03.10 Merge rc v1.6.0 feat pubkey conv 2023.03.10

cmd/keygenerator/converter/pidPubkeyConverter.go

config/tomlConfig_test.go

iulianpascalau · 2023-03-13T08:01:50Z

go.mod

@@ -13,13 +13,13 @@ require (
 	github.com/google/gops v0.3.18
 	github.com/gorilla/websocket v1.5.0
 	github.com/mitchellh/mapstructure v1.5.0
-	github.com/multiversx/mx-chain-core-go v1.1.35
+	github.com/multiversx/mx-chain-core-go v1.1.36-0.20230308081722-5262fb09cb9a


proper releases before merging

immediately after review and system test 👍

integrationTests/consensus/consensus_test.go

node/external/transactionAPI/apiTransactionProcessor.go

outport/process/alteredaccounts/alteredAccountsProvider.go

state/accountsDB.go

andreibancioiu · 2023-03-13T15:43:46Z

cmd/keygenerator/main.go

@@ -41,6 +41,7 @@ const minedWalletPrefixKeys = "mined-wallet"
 const nopattern = "nopattern"
 const desiredpattern = "[0-f]+"
 const noshard = -1
+const pubkeyHrp = "erd"


In the future, this can be received as a command line parameter.

...or a config defined in config.toml file

andreibancioiu · 2023-03-13T17:08:59Z

state/syncer/userAccountsSyncer.go

@@ -288,9 +288,9 @@ func (u *userAccountsSyncer) printDataTrieStatistics() {
 	})

 	for _, trieStat := range u.largeTries {
-		address := u.pubkeyCoverter.Encode(trieStat.address)
+		trieStatAddress := u.pubkeyCoverter.SilentEncode(trieStat.address, log)


Previous name is better (address).

andreibancioiu · 2023-03-13T17:12:22Z

process/smartContract/process.go

@@ -1800,11 +1800,15 @@ func (sc *scProcessor) printScDeployed(vmOutput *vmcommon.VMOutput, tx data.Tran
 			continue
 		}

-		scGenerated = append(scGenerated, sc.pubkeyConv.Encode(addr))
+		scAddress := sc.pubkeyConv.SilentEncode(addr, log)
+


Empty line is not necessary here.

andreibancioiu · 2023-03-13T17:13:34Z

process/smartContract/process.go

 	log.Debug("SmartContract deployed",
-		"owner", sc.pubkeyConv.Encode(tx.GetSndAddr()),
+		"owner", encodedSndAddr,


In the log on line 432, the encoding is "in-lined". Perhaps do the same here, we well?

Or, instead, perhaps rename variable to ownerAddress?

andreibancioiu · 2023-03-13T17:28:20Z

node/node_test.go

@@ -875,7 +876,7 @@ func TestNode_GetAllESDTTokensShouldReturnEsdtAndFormattedNft(t *testing.T) {
 	tokens, _, err := n.GetAllESDTTokens(testscommon.TestAddressAlice, api.AccountQueryOptions{}, context.Background())
 	assert.Nil(t, err)
 	assert.Equal(t, 2, len(tokens))
-	assert.Equal(t, esdtData, tokens[esdtToken])
+	assert.Equal(t, esdtData, tokens["TKKR-7q8w9e-"])


I am not sure what happened here.

reverted in the next PR :D

andreibancioiu · 2023-03-13T17:36:13Z

integrationTests/frontend/wallet/dataField_test.go

+	converter, _ := pubkeyConverter.NewBech32PubkeyConverter(32, "erd")
+
+	receiverAddress, _ := converter.Encode(tx.RcvAddr)
+


Empty lines not strictly necessary here.

andreibancioiu · 2023-03-13T18:03:22Z

node/external/logs/logsConverter.go

 		Events:  events,
 	}
 }

 func (converter *logsConverter) encodeAddress(pubkey []byte) string {
-	return converter.pubKeyConverter.Encode(pubkey)
+	return converter.pubKeyConverter.SilentEncode(pubkey, log)


andreibancioiu · 2023-03-13T18:30:37Z

process/peer/process.go

-	leaderPK := core.GetTrimmedPk(vs.pubkeyConv.Encode(consensusGroup[0].PubKey()))
+
+	encodedLeaderPk := vs.pubkeyConv.SilentEncode(consensusGroup[0].PubKey(), log)
+


Empty line is not necessary, I think.

andreibancioiu · 2023-03-13T18:41:54Z

outport/process/alteredaccounts/alteredAccountsProvider.go

+		return nil, nil
+	}
+
+	metaDataCreatorAddr, err := aap.addressConverter.Encode(metaData.Creator)


Are we sure this should not be a silent encode? CC: @bogdan-rosianu.

andreibancioiu · 2023-03-13T18:44:18Z

node/trieIterators/delegatedListProcessor.go

@@ -96,8 +96,13 @@ func (dlp *delegatedListProcessor) getDelegatorsInfo(delegationSC []byte, delega

 		delegatorInfo, ok := delegatorsMap[string(delegatorAddress)]
 		if !ok {
+			encodedDelegatorAddress, err := dlp.publicKeyConverter.Encode(delegatorAddress)


Are we sure we don't need silent encode in this file, instead?

Fix after review pubkeyconverter

Merge branch 'rc/v1.6.0' into pubkey_15Mar

# Conflicts: # go.mod # go.sum # outport/process/transactionsfee/transactionsFeeProcessor_test.go

…bkeyconv-mar15 Merge rc/v1.6.0 into feat/pubkeyconv mar15

update core-go, vm-common-go, indexer-go versions

gabi-vuls

System test passed

iulianpascalau

go.mod looks ok 👌

schimih added 10 commits November 9, 2022 12:47

pubkeyConverter refactor

2ad1ee5

pubkeyConverter refactor

c8213c2

pubkeyconverter refactor - process

707c83d

pubkeyConverter refactor - node package

6f9b731

pubkeyConverter refactor - update package

c0da058

pubkeyconverter refactor - sharding, vm, outport

da583fa

fix

3e02ebc

Merge branch 'rc/v1.4.0' into EN-10782-Change_bech

a9f7df6

small fixes for tests

3646d0b

update interf, mocks & stubs after go-core change

aa40318

schimih self-assigned this Nov 18, 2022

schimih marked this pull request as draft November 18, 2022 16:38

Merge branch 'feat/pubkeyconverter-refactor' into EN-10782-Change_bec…

db39042

…h32_addresses_format

schimih added interested:wallet size:small type:refactoring priority:4-!i&!u Not Important and not Urgent labels Dec 6, 2022

schimih and others added 7 commits December 7, 2022 15:33

update

45487da

update with SilentEncode instead of QuietEncode

525f72e

remove withMetaData

8d64677

Merge branch 'rc/v1.5.0' into EN-10782-Change_bech32_addresses_format

b9e3ee4

lint fix

3fc0a2f

fix linter issues

cc27371

Merge pull request #4778 from ElrondNetwork/merge-rcv1.5.0-2022.12.08

7600128

Merge rcv1.5.0 2022.12.08 to pubkeyconvert-refactor

schimih changed the base branch from master to rc/v1.5.0 December 9, 2022 08:58

Merge branch 'feat/pubkeyconverter-refactor' into EN-10782-Change_bec…

6e88690

…h32_addresses_format

schimih added 3 commits December 9, 2022 15:27

fix after review

d6a2d6b

fix after self review

e26c573

Merge branch 'rc/v1.5.0' into EN-10782-Change

748983a

schimih added 4 commits February 23, 2023 15:54

Merge pull request #5025 from multiversx/merge_rc/v1.6.0_in_pubkeycon…

543dc13

…v_23feb merge rc/v1.6.0 in pubkeyConverter

Merge branch 'rc/v1.6.0' into feat/pubkey_conv

6eb4dfb

fix lint issue

61ad141

Merge pull request #5066 from multiversx/merge_rc1.6.0_in_pubkey

ce1aa36

Merge branch 'rc/v1.6.0' into feat/pubkey_conv

andreibancioiu changed the base branch from rc/v1.6.0 to master March 9, 2023 13:38

andreibancioiu changed the base branch from master to rc/v1.4.0 March 9, 2023 13:39

andreibancioiu changed the base branch from rc/v1.4.0 to rc/v1.5.0 March 9, 2023 13:39

andreibancioiu changed the base branch from rc/v1.5.0 to rc/v1.6.0 March 9, 2023 13:39

iulianpascalau and others added 3 commits March 10, 2023 13:42

- fixes after merge

c19861f

Merge pull request #5084 from multiversx/merge-rc-v1.6.0-feat-pubkey-…

b559b2a

…conv-2023.03.10 Merge rc v1.6.0 feat pubkey conv 2023.03.10

iulianpascalau self-requested a review March 13, 2023 07:45

iulianpascalau requested changes Mar 13, 2023

View reviewed changes

fix after review

8bba982

andreibancioiu reviewed Mar 13, 2023

View reviewed changes

schimih and others added 9 commits March 14, 2023 14:02

fixes after review 2

85d4d15

fix after 2nd review and update indexer

34f411f

Merge pull request #5098 from multiversx/fix_pubkeyconverter

cdc7eee

Fix after review pubkeyconverter

Merge branch 'rc/v1.6.0' into pubkey_15Mar

fc501eb

Merge pull request #5100 from multiversx/merge_rc/v1.6.0_in_pubkey_15Mar

efb5334

Merge branch 'rc/v1.6.0' into pubkey_15Mar

Merge branch 'rc/v1.6.0' into merge-rc/v1.6.0-into-feat/pubkeyconv-mar15

1c3ce42

# Conflicts: # go.mod # go.sum # outport/process/transactionsfee/transactionsFeeProcessor_test.go

Merge pull request #5104 from multiversx/merge-rc/v1.6.0-into-feat/pu…

58c40b0

…bkeyconv-mar15 Merge rc/v1.6.0 into feat/pubkeyconv mar15

update core-go, vm-common-go, indexer-go versions

27733cd

Merge pull request #5109 from multiversx/update_go_mod

3848ddb

update core-go, vm-common-go, indexer-go versions

schimih marked this pull request as ready for review March 20, 2023 11:38

gabi-vuls approved these changes Mar 20, 2023

View reviewed changes

iulianpascalau approved these changes Mar 20, 2023

View reviewed changes

andreibancioiu approved these changes Mar 20, 2023

View reviewed changes

schimih merged commit e272723 into rc/v1.6.0 Mar 20, 2023

schimih deleted the feat/pubkeyconverter-refactor branch March 20, 2023 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/pubkeyconverter_refactor #4716

Feat/pubkeyconverter_refactor #4716

schimih commented Nov 18, 2022 •

edited

Loading

codecov-commenter commented Dec 9, 2022 •

edited

Loading

iulianpascalau Mar 13, 2023

schimih Mar 13, 2023

andreibancioiu Mar 13, 2023

schimih Mar 13, 2023

iulianpascalau Mar 15, 2023

andreibancioiu Mar 13, 2023

schimih Mar 13, 2023

andreibancioiu Mar 13, 2023

schimih Mar 13, 2023

andreibancioiu Mar 13, 2023

schimih Mar 13, 2023

andreibancioiu Mar 13, 2023

iulianpascalau Mar 15, 2023

andreibancioiu Mar 13, 2023

andreibancioiu Mar 13, 2023

andreibancioiu Mar 13, 2023

andreibancioiu Mar 13, 2023

andreibancioiu Mar 13, 2023

gabi-vuls left a comment

iulianpascalau left a comment

		converter, _ := pubkeyConverter.NewBech32PubkeyConverter(32, "erd")

		receiverAddress, _ := converter.Encode(tx.RcvAddr)

		leaderPK := core.GetTrimmedPk(vs.pubkeyConv.Encode(consensusGroup[0].PubKey()))

		encodedLeaderPk := vs.pubkeyConv.SilentEncode(consensusGroup[0].PubKey(), log)

Feat/pubkeyconverter_refactor #4716

Feat/pubkeyconverter_refactor #4716

Conversation

schimih commented Nov 18, 2022 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Dec 9, 2022 • edited Loading

Codecov Report

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

iulianpascalau left a comment

Choose a reason for hiding this comment

schimih commented Nov 18, 2022 •

edited

Loading

codecov-commenter commented Dec 9, 2022 •

edited

Loading