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

MX-14120: repopulate tokens supplies #5204

Merged
merged 17 commits into from May 16, 2023

Conversation

bogdan-rosianu
Copy link
Contributor

@bogdan-rosianu bogdan-rosianu commented Apr 27, 2023

Reasoning behind the pull request

  • previously, tokens supplies could have only been re-computed via an import-db which could have taken a few days, so we needed a faster mechanism for computing the tokens supplies

Proposed changes

  • add a CLI flag --repopulate-tokens-supplies that will clear the existing supplies database, will iterate over the trie and recompute the supplies and then save them to the db
    NOTE: this feature does not compute the minted and burned values. They will be later indexed via the indexer

Testing procedure

  • run an observing squad on testnet/devnet/mainnet by starting the nodes with the --repopulate-tokens-supplies and then compare the supplies obtained by comparing the proxy instances via the /network/esdt/supply/<token> endpoint

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?

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 80.95% and project coverage change: +0.10 🎉

Comparison is base (ffd2cb0) 79.59% compared to head (b5c34c8) 79.70%.

❗ Current head b5c34c8 differs from pull request most recent head 7029a48. Consider uploading reports for the commit 7029a48 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.6.0    #5204      +/-   ##
=============================================
+ Coverage      79.59%   79.70%   +0.10%     
=============================================
  Files            680      682       +2     
  Lines          88012    88183     +171     
=============================================
+ Hits           70053    70283     +230     
+ Misses         12837    12780      -57     
+ Partials        5122     5120       -2     
Impacted Files Coverage Δ
facade/nodeFacade.go 97.25% <ø> (+18.77%) ⬆️
process/sync/baseSync.go 64.91% <9.52%> (-1.71%) ⬇️
factory/consensus/consensusComponents.go 96.24% <66.66%> (-0.39%) ⬇️
process/sync/shardblock.go 67.28% <66.66%> (-0.18%) ⬇️
storage/factory/storageServiceFactory.go 89.48% <68.00%> (-1.86%) ⬇️
factory/processing/processComponents.go 83.01% <83.33%> (-0.03%) ⬇️
...cess/sync/trieIterators/tokensSuppliesProcessor.go 89.87% <89.87%> (ø)
epochStart/bootstrap/metaStorageHandler.go 73.97% <100.00%> (+0.17%) ⬆️
epochStart/bootstrap/process.go 84.80% <100.00%> (+0.01%) ⬆️
epochStart/bootstrap/shardStorageHandler.go 88.00% <100.00%> (+0.02%) ⬆️
... and 7 more

... and 2 files with indirect coverage changes

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

@bogdan-rosianu bogdan-rosianu self-assigned this Apr 27, 2023
@bogdan-rosianu bogdan-rosianu marked this pull request as ready for review April 27, 2023 14:11
@bogdan-rosianu bogdan-rosianu marked this pull request as draft April 28, 2023 12:16
@bogdan-rosianu bogdan-rosianu marked this pull request as ready for review May 2, 2023 14:12
@iulianpascalau iulianpascalau self-requested a review May 2, 2023 14:27
@@ -13,7 +13,7 @@ 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.2.1
github.com/multiversx/mx-chain-core-go v1.2.2-0.20230428142157-76e19ecd04ac
Copy link
Contributor

Choose a reason for hiding this comment

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

proper release after testing

@@ -1227,7 +1228,7 @@ func (nr *nodeRunner) CreateManagedProcessComponents(
ImportStartHandler: importStartHandler,
WorkingDir: configs.FlagsConfig.WorkingDir,
HistoryRepo: historyRepository,
SnapshotsEnabled: configs.FlagsConfig.SnapshotsEnabled,
FlagsConfig: *configs.FlagsConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not pass config by value? I consider this a safer way to pass arguments so no overwriting mistakes will have effects upstream. Also, WorkingDir also uses the configs.FlagsConfig.WorkingDir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already passed by value. updated the other fields

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

process/sync/shardblock.go Outdated Show resolved Hide resolved
suffix := append(userLeaf.Key(), userAccount.AddressBytes()...)
value, errVal := userLeaf.ValueWithoutSuffix(suffix)
if errVal != nil {
log.Warn("cannot get value without suffix", "error", errVal, "key", userLeaf.Key())
Copy link
Contributor

Choose a reason for hiding this comment

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

log & return 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.

removed the log and wrapped the error

process/sync/baseSync.go Outdated Show resolved Hide resolved
process/sync/trieIterators/tokensSuppliesComputer.go Outdated Show resolved Hide resolved
return err
}

tokenName := string(tokenKey)[lenESDTPrefix:]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we test here that the tokenKey contains the ESDTPrefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need, since we have this at the beginning of the for loop:

if !bytes.HasPrefix(userLeaf.Key(), esdtPrefix) {
			continue
		}

and also this: lenESDTPrefix := len(esdtPrefix)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

process/sync/trieIterators/tokensSuppliesComputer.go Outdated Show resolved Hide resolved
process/sync/trieIterators/tokensSuppliesComputer_test.go Outdated Show resolved Hide resolved
@@ -136,7 +136,10 @@ func (ccf *consensusComponentsFactory) Create() (*consensusComponents, error) {
return nil, err
}

cc.bootstrapper.StartSyncingBlocks()
err = cc.bootstrapper.StartSyncingBlocks()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -173,7 +171,6 @@ type processComponentsFactory struct {
txLogsProcessor process.TransactionLogProcessor
version string
importStartHandler update.ImportStartHandler
workingDir string
Copy link
Contributor

Choose a reason for hiding this comment

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

we removed the workingDir but not the version. The code will compile with an empty version string as you delete L221. Let's also remove this field and use the provided version 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.

very good catch. fixed

@@ -1227,7 +1228,7 @@ func (nr *nodeRunner) CreateManagedProcessComponents(
ImportStartHandler: importStartHandler,
WorkingDir: configs.FlagsConfig.WorkingDir,
HistoryRepo: historyRepository,
SnapshotsEnabled: configs.FlagsConfig.SnapshotsEnabled,
FlagsConfig: *configs.FlagsConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

return err
}

tokenName := string(tokenKey)[lenESDTPrefix:]
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

userAcc.SetDataTrie(&trie.TrieStub{
GetAllLeavesOnChannelCalled: func(leavesChannels *common.TrieIteratorChannels, ctx context.Context, rootHash []byte, keyBuilder common.KeyBuilder) error {
esToken := &esdt.ESDigitalToken{
Value: big.NewInt(37),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

iulianpascalau
iulianpascalau previously approved these changes May 4, 2023
@sstanculeanu sstanculeanu self-requested a review May 5, 2023 08:26
"github.com/multiversx/mx-chain-go/trie/keyBuilder"
)

type tokensSuppliesProcessor struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

tokenIDStr := string(tokenID)
if nonce > 0 {
t.putInSuppliesMap(string(tokenID), value) // put for collection as well
nonceStr := fmt.Sprintf("%d", nonce)
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed

Suggested change
nonceStr := fmt.Sprintf("%d", nonce)
nonceStr := hex.EncodeToString(big.NewInt(int64(nonce)).Bytes())

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

@@ -0,0 +1,245 @@
package trieIterators
Copy link
Contributor

Choose a reason for hiding this comment

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

code coverage can be increased to close to 100% on this package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

96.2% now

go boot.syncBlocks(ctx)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

add new line before?

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

@@ -12,4 +12,5 @@ message SupplyESDT {
bytes Supply = 1 [(gogoproto.jsontag) = "value", (gogoproto.casttypewith) = "math/big.Int;github.com/multiversx/mx-chain-core-go/data.BigIntCaster"];
bytes Burned = 2 [(gogoproto.jsontag) = "burned", (gogoproto.casttypewith) = "math/big.Int;github.com/multiversx/mx-chain-core-go/data.BigIntCaster"];
bytes Minted = 3 [(gogoproto.jsontag) = "minted", (gogoproto.casttypewith) = "math/big.Int;github.com/multiversx/mx-chain-core-go/data.BigIntCaster"];
bool RecomputedSupply = 4 [(gogoproto.jsontag) = "RecomputedSupply"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool RecomputedSupply = 4 [(gogoproto.jsontag) = "RecomputedSupply"];
bool RecomputedSupply = 4 [(gogoproto.jsontag) = "recomputedSupply"];

to align with the others

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 find. fixed

sstanculeanu
sstanculeanu previously approved these changes May 5, 2023
iulianpascalau
iulianpascalau previously approved these changes May 5, 2023
gabi-vuls
gabi-vuls previously approved these changes May 16, 2023
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed

@bogdan-rosianu bogdan-rosianu merged commit 360ed29 into rc/v1.6.0 May 16, 2023
5 checks passed
@bogdan-rosianu bogdan-rosianu deleted the MX-14120-repopulate-tokens-supplies branch May 16, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants