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

fix(drive)!: non-deterministic behaviour in masternode identities logic #287

Merged
merged 46 commits into from
Mar 10, 2022

Conversation

shumkov
Copy link
Member

@shumkov shumkov commented Mar 9, 2022

Issue being fixed or feature implemented

The creation of masternode reward share documents is not deterministic which leads to a chain halt.
There is a lack of tests so many issues were visible on only testnet.

What was done?

  • Created the first masternode with 10% of operatorReward in a local network (dashmate)
  • Fixed creating a reward share document if already exists with the same $ownerId and payToId pair
  • Fixed non-deterministic ID of reward share documents
  • Fixed operator identifier
  • Fixed missing await
  • Fixed removing only 100 reward shares
  • Refactored document creation to use repositories directly
  • Implement integration test to catch all cases in masternode identities logic
  • Remove source of non-determinism (random system contract pub keys) for test runs

How Has This Been Tested?

With integration test

Breaking Changes

The fixed masternode identities logic breaks compatibility with previous invalid state.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging 51e6f7d into d920064 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging 3e21553 into d920064 - view on LGTM.com

new alerts:

  • 1 for Syntax error

shumkov and others added 23 commits March 9, 2022 22:24
…e-identities' into fix-non-determinism-in-masternode-identities

# Conflicts:
#	packages/js-drive/test/integration/identity/masternode/synchronizeMasternodeIdentitiesFactory.spec.js
….com:dashevo/platform into fix-non-determinism-in-masternode-identities
…e-identities' into fix-non-determinism-in-masternode-identities

# Conflicts:
#	packages/js-drive/test/integration/identity/masternode/synchronizeMasternodeIdentitiesFactory.spec.js
….com:dashevo/platform into fix-non-determinism-in-masternode-identities
…e-identities' into fix-non-determinism-in-masternode-identities
…e-identities' into fix-non-determinism-in-masternode-identities
jawid-h and others added 5 commits March 9, 2022 22:17
….com:dashevo/platform into fix-non-determinism-in-masternode-identities
….com:dashevo/platform into fix-non-determinism-in-masternode-identities
@jawid-h jawid-h marked this pull request as ready for review March 9, 2022 17:52
@jawid-h jawid-h requested a review from antouhou as a code owner March 9, 2022 17:52
@jawid-h jawid-h changed the title fix: non-deterministic behaviour in masternode identities fix(drive): non-deterministic behaviour in masternode identities logic Mar 9, 2022
@shumkov shumkov changed the title fix(drive): non-deterministic behaviour in masternode identities logic fix: non-deterministic behaviour in masternode identities logic Mar 10, 2022
@shumkov shumkov changed the title fix: non-deterministic behaviour in masternode identities logic fix(drive): non-deterministic behaviour in masternode identities logic Mar 10, 2022
@shumkov shumkov changed the title fix(drive): non-deterministic behaviour in masternode identities logic fix(drive)!: non-deterministic behaviour in masternode identities logic Mar 10, 2022
try {
({ result: rawTransaction } = await coreRpcClient.getRawTransaction(id, 1));
} catch (e) {
// Invalid address or key error
Copy link
Member

Choose a reason for hiding this comment

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

I think if it times out you should retry. Not essential for this version but still something todo.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. We have something about connection reliability with Core in our backlog.

Copy link
Collaborator

@pshenmic pshenmic left a comment

Choose a reason for hiding this comment

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

Nice work!

@shumkov shumkov merged commit 74818f6 into v0.22-dev Mar 10, 2022
@shumkov shumkov deleted the fix-non-determinism-in-masternode-identities branch March 10, 2022 06:14
@thephez thephez added this to the v0.22.0 milestone Mar 15, 2022
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.

5 participants