fix: Resolve mainnet v19 fork issues by UdjinM6 · Pull Request #5403 · dashpay/dash

UdjinM6 · 2023-05-30T21:02:13Z

Issue being fixed or feature implemented

same as #5392, alternative solution

~~based on #5402 atm, will rebase later~~

What was done?

pls see individual commits

How Has This Been Tested?

reorg mainnet around forkpoint with a patched client (to allow low difficulty), run tests

Breaking Changes

Another evodb migration is required. Going back to an older version or migrating after the fork requires reindexing.

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
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

ogabrielides

LGTM.

Tested evoDB migration from 18.2 and from 19.1.
Tested v19 forking on Mainnet with --noconnect and fPowNoRetargeting=true back and forth.
Tested the problematic scenario on regtest with disabled caching for mnLists.

utACK

…nd not bls::bls_legacy_scheme

…uf is set via ToByteVector

…deser pubKeyOperator

… properties in any way

…icMNState/CPro_Tx serialization

PastaPastaPasta

ACK for squash merge

…nisticMNState Member obj.keyIDOwner is read & write twice

…State (#5413) ## Issue being fixed or feature implemented Member obj.keyIDOwner is read & write twice ## What was done? Fixed: it is serialized once ## How Has This Been Tested? Unit/functional tests in CI ## Breaking Changes Data format in database changed in incompatible way ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone

@HashEngineering

…/deser pubKeyOperator (#5397) ## Issue being fixed or feature implemented Mobile wallets would have to convert 4k+ pubkeys at the V19 fork point and it's a pretty hard job for them that can easily take 10-15 seconds if not more. Also after the HF, if a masternode list is requested from before the HF, the operator keys come in basic scheme, but the merkelroot was calculated with legacy. From mobile team work it wasn't possible to convert all operator keys to legacy and then calculate the correct merkleroot. ~This PR builds on top of ~#5392~ #5403 (changes that belong to this PR: 26f7e966500bdea4c604f1d16716b40b366fc707 and 4b42dc8fcee3354afd82ce7e3a72ebe1659f5f22) and aims to solve both of these issues.~ cc @HashEngineering @QuantumExplorer ## What was done? Introduce `nVersion` on p2p level for every CSimplifiedMNListEntry. Set `nVersion` to the same value we have it in CDeterministicMNState i.e. pubkey serialization would not be via basic scheme only after the V19 fork, it would match the way it’s serialized on-chain/in CDeterministicMNState for that specific MN. ## How Has This Been Tested? run tests ## Breaking Changes NOTE: `testnet` is going to re-fork at v19 forkpoint because `merkleRootMNList` is not going to match ## Checklist: - [x] 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 - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

same as dashpay#5392, alternative solution ~based on dashpay#5402 atm, will rebase later~ pls see individual commits reorg mainnet around forkpoint with a patched client (to allow low difficulty), run tests Another evodb migration is required. Going back to an older version or migrating after the fork requires reindexing. - [x] I have performed a self-review of my own code - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

…isticMNState (dashpay#5413) ## Issue being fixed or feature implemented Member obj.keyIDOwner is read & write twice ## What was done? Fixed: it is serialized once ## How Has This Been Tested? Unit/functional tests in CI ## Breaking Changes Data format in database changed in incompatible way ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone

@HashEngineering

…/deser pubKeyOperator (dashpay#5397) Mobile wallets would have to convert 4k+ pubkeys at the V19 fork point and it's a pretty hard job for them that can easily take 10-15 seconds if not more. Also after the HF, if a masternode list is requested from before the HF, the operator keys come in basic scheme, but the merkelroot was calculated with legacy. From mobile team work it wasn't possible to convert all operator keys to legacy and then calculate the correct merkleroot. ~This PR builds on top of ~dashpay#5392~ dashpay#5403 (changes that belong to this PR: 26f7e966500bdea4c604f1d16716b40b366fc707 and 4b42dc8fcee3354afd82ce7e3a72ebe1659f5f22) and aims to solve both of these issues.~ cc @HashEngineering @QuantumExplorer Introduce `nVersion` on p2p level for every CSimplifiedMNListEntry. Set `nVersion` to the same value we have it in CDeterministicMNState i.e. pubkey serialization would not be via basic scheme only after the V19 fork, it would match the way it’s serialized on-chain/in CDeterministicMNState for that specific MN. run tests NOTE: `testnet` is going to re-fork at v19 forkpoint because `merkleRootMNList` is not going to match - [x] 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 - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

@HashEngineering

…/deser pubKeyOperator (dashpay#5397) Mobile wallets would have to convert 4k+ pubkeys at the V19 fork point and it's a pretty hard job for them that can easily take 10-15 seconds if not more. Also after the HF, if a masternode list is requested from before the HF, the operator keys come in basic scheme, but the merkelroot was calculated with legacy. From mobile team work it wasn't possible to convert all operator keys to legacy and then calculate the correct merkleroot. ~This PR builds on top of ~dashpay#5392~ dashpay#5403 (changes that belong to this PR: 26f7e966500bdea4c604f1d16716b40b366fc707 and 4b42dc8fcee3354afd82ce7e3a72ebe1659f5f22) and aims to solve both of these issues.~ cc @HashEngineering @QuantumExplorer Introduce `nVersion` on p2p level for every CSimplifiedMNListEntry. Set `nVersion` to the same value we have it in CDeterministicMNState i.e. pubkey serialization would not be via basic scheme only after the V19 fork, it would match the way it’s serialized on-chain/in CDeterministicMNState for that specific MN. run tests NOTE: `testnet` is going to re-fork at v19 forkpoint because `merkleRootMNList` is not going to match - [x] 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 - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

## Issue being fixed or feature implemented Many usages of `CBLS{Signature,PrivateKey,PublicKey}` assume using global variable, even if can be specified explicitly. Some of these usages have been deglobalized in this PR. Some prior improvements and fixes are here: [#5403](#5403) ## What was done? - Refactored the uses of global variable of `bls_legacy_scheme` from `SetHex`, `SetByteVector`, some rpc calls. - Removed flag `checkMalleable` to simplify code because it's always `true`. - Removed dependency from `txmempool.h` on `bls.h` to speed up compilation. ## How Has This Been Tested? Run unit/functional tests. ## Breaking Changes No breaking changes assumed. But in theory behaviour of some RPC can be more explicit and predictable. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone

…b upgrade logic 39f76c8 chore: drop legacy `CDeterministicMNState` formats, evodb upgrade logic (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * `CDeterministicMNState_`{`Oldformat`, `mntype_format`} was introduced in [dash#5039](#5039) and [dash#5403](#5403), included in Dash Core v19 and v20 respectively. The above change has been in the codebase for more than two major releases and does not concern databases with a long shelf life (like wallets), this pull request removes them entirely. ## Breaking Changes Users upgrading from these versions are now expected to `-reindex`. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 39f76c8 knst: utACK 39f76c8 Tree-SHA512: 3fabe0aeb7f3d01a858b0b19d49a22523f462157597a1ade93181288e4ba7b133a5ee9272a8abe41e40cff177c0ddf94123274f0427693db6fdb90c4b8e613fa

UdjinM6 added the backport-candidate-23.1.x label May 30, 2023

UdjinM6 modified the milestones: 20, 19.2 May 30, 2023

UdjinM6 mentioned this pull request May 31, 2023

fix: Resolve mainnet v19 fork issues #5392

Closed

5 tasks

ogabrielides previously approved these changes May 31, 2023

View reviewed changes

UdjinM6 added 9 commits May 31, 2023 23:35

fix: Rework bls_legacy_scheme switch points

3903421

chore: drop RepopulateUniquePropertyMap

daf9ff9

feat: let CBLSLazyWrapper follow the scheme it was created/set with a…

34b5465

…nd not bls::bls_legacy_scheme

fix: set bufValid in CBLSLazyWrapper to true only on unser and when b…

bc6045e

…uf is set via ToByteVector

feat: store protx version in CDeterministicMNState and use it to ser/…

499d0fa

…deser pubKeyOperator

feat: make pubKeyOperator in CPro_Tx-es lazy

8a4e859

feat: make sure non-lazy CBLSPublicKeys aren't used in mn list unique…

1b00ba1

… properties in any way

feat: show pubKeyOperator according to nVersion to match CDeterminist…

949075c

…icMNState/CPro_Tx serialization

test: check pubkeyoperator in masternodelist rpc

965e7cf

UdjinM6 force-pushed the fix_fork_3 branch from 4747546 to 965e7cf Compare May 31, 2023 20:35

UdjinM6 marked this pull request as ready for review May 31, 2023 20:36

UdjinM6 requested a review from PastaPastaPasta May 31, 2023 20:36

test: add v19 activation unit test

0f6d110

UdjinM6 dismissed ogabrielides’s stale review via 0f6d110 June 3, 2023 17:44

UdjinM6 mentioned this pull request Jun 3, 2023

test: add v19 activation unit test #5411

Closed

5 tasks

PastaPastaPasta approved these changes Jun 4, 2023

View reviewed changes

UdjinM6 merged commit 54fb76f into dashpay:develop Jun 4, 2023

UdjinM6 deleted the fix_fork_3 branch June 4, 2023 20:46

UdjinM6 mentioned this pull request Jun 4, 2023

feat: store protx version in CSimplifiedMNListEntry and use it to ser/deser pubKeyOperator #5397

Merged

5 tasks

knst added a commit to knst/dash that referenced this pull request Jun 5, 2023

fix: follow-up dashpay#5403 - remove double serialization in CDetermi…

b76ec47

…nisticMNState Member obj.keyIDOwner is read & write twice

UdjinM6 removed the backport-candidate-23.1.x label Jun 13, 2023

knst mentioned this pull request Jun 19, 2023

refactor: deglobalization of bls_legacy_scheme 2/N #5443

Merged

5 tasks

This was referenced Feb 10, 2025

chore: drop deprecated RPCs masternode current, masternode winner and getpoolinfo #6567

Merged

chore: drop legacy CDeterministicMNState formats, evodb upgrade logic #6579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Resolve mainnet v19 fork issues#5403

fix: Resolve mainnet v19 fork issues#5403
UdjinM6 merged 10 commits intodashpay:developfrom
UdjinM6:fix_fork_3

UdjinM6 commented May 30, 2023 •

edited

Loading

Uh oh!

ogabrielides left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

UdjinM6 commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

UdjinM6 commented May 30, 2023 •

edited

Loading