Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented May 30, 2023

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
ogabrielides previously approved these changes May 31, 2023
Copy link

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

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

@UdjinM6 UdjinM6 marked this pull request as ready for review May 31, 2023 20:36
@UdjinM6 UdjinM6 requested a review from PastaPastaPasta May 31, 2023 20:36
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

ACK for squash merge

@UdjinM6 UdjinM6 merged commit 54fb76f into dashpay:develop Jun 4, 2023
@UdjinM6 UdjinM6 deleted the fix_fork_3 branch June 4, 2023 20:46
knst added a commit to knst/dash that referenced this pull request Jun 5, 2023
…nisticMNState

Member obj.keyIDOwner is read & write twice
PastaPastaPasta pushed a commit that referenced this pull request Jun 6, 2023
…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
PastaPastaPasta pushed a commit that referenced this pull request Jun 11, 2023
…/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)_
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 11, 2023
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)_
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 11, 2023
…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
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 11, 2023
…/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)_
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 12, 2023
…/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)_
PastaPastaPasta pushed a commit that referenced this pull request Jul 1, 2023
## 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
PastaPastaPasta added a commit that referenced this pull request Feb 16, 2025
…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
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.

3 participants