Skip to content

Conversation

@miiu96
Copy link
Contributor

@miiu96 miiu96 commented Mar 13, 2025

Reasoning behind the pull request

  • New integration test to check if rewards are distributed to all shards

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?

@miiu96 miiu96 self-assigned this Mar 13, 2025
defaultPathToInitialConfig = "../../../cmd/node/config/"
)

func TestRewardsTxsAfterEquivalentMessages(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we consider this a long test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, changed

ApiInterface: api.NewNoApiInterface(),
MinNodesPerShard: 3,
MetaChainMinNodes: 3,
AlterConfigsFunction: func(cfg *config.Configs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove new line?

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

Comment on lines 56 to 65
for i := 0; i < targetEpoch; i++ {
if i == 8 {
fmt.Println("here")
}
err = cs.ForceChangeOfEpoch()
require.Nil(t, err)
}

err = cs.GenerateBlocks(210)
require.Nil(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed? or GenerateBlocksUntilEpochIsReached can be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use that function, more blocks will be generated until I reach epoch 9, with this method, only 30 blocks are generated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe remove

if i == 8 {
	fmt.Println("here")
}

nodesSetupFile := path.Join(tempDir, "config", "nodesSetup.json")
validators, err := readValidatorsAndOwners(nodesSetupFile)
require.Nil(t, err)
fmt.Println(validators)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed?

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, removed

rewardsPerShard[shardID] = big.NewInt(0)
}

valueBig, okR := rewardsPerShard[shardID].SetString(tx.Value, 10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
valueBig, okR := rewardsPerShard[shardID].SetString(tx.Value, 10)
valueBig, okR := big.NewInt(0).SetString(tx.Value, 10)

Copy link
Collaborator

Choose a reason for hiding this comment

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

otherwise it will also set rewardsPerShard[shardID]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

func allValuesEqual(m map[uint32]*big.Int) bool {
var firstValue *big.Int
Copy link
Collaborator

Choose a reason for hiding this comment

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

can use m[0] as expectedValue instead of these 2 variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

}

func (s *simulator) addProofs() {
proofs := make([]*block.HeaderProof, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
proofs := make([]*block.HeaderProof, 0)
proofs := make([]*block.HeaderProof, 0, len(s.nodes))

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

}

for _, proof := range proofs {
s.GetNodeHandler(core.MetachainShardId).GetDataComponents().Datapool().Proofs().AddProof(proof)
Copy link
Collaborator

Choose a reason for hiding this comment

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

extract s.GetNodeHandler(core.MetachainShardId).GetDataComponents().Datapool().Proofs() outside of the for loop

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

Comment on lines 56 to 65
for i := 0; i < targetEpoch; i++ {
if i == 8 {
fmt.Println("here")
}
err = cs.ForceChangeOfEpoch()
require.Nil(t, err)
}

err = cs.GenerateBlocks(210)
require.Nil(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe remove

if i == 8 {
	fmt.Println("here")
}

Comment on lines 166 to 182
func allValuesEqual(m map[uint32]*big.Int) bool {
if len(m) == 0 {
return true
}

var firstValue *big.Int
for _, v := range m {
if firstValue == nil {
firstValue = v
continue
}
if firstValue.Cmp(v) != 0 {
return false
}
}
return true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func allValuesEqual(m map[uint32]*big.Int) bool {
if len(m) == 0 {
return true
}
var firstValue *big.Int
for _, v := range m {
if firstValue == nil {
firstValue = v
continue
}
if firstValue.Cmp(v) != 0 {
return false
}
}
return true
}
func allValuesEqual(m map[uint32]*big.Int) bool {
if len(m) == 0 {
return true
}
expectedValue := m[0]
for _, v := range m {
if expectedValue.Cmp(v) != 0 {
return false
}
}
return true
}

this was my initial suggestion, assuming we always have a shard 0

sstanculeanu
sstanculeanu previously approved these changes Mar 14, 2025
@miiu96 miiu96 merged commit bd1cf2a into feat/andromeda-fixes Mar 17, 2025
4 checks passed
@miiu96 miiu96 deleted the integration-test-rewards branch March 17, 2025 08:25
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.

4 participants