-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: Resolve mainnet v19 fork issues #5403
Conversation
There was a problem hiding this 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
…nd not bls::bls_legacy_scheme
…uf is set via ToByteVector
…deser pubKeyOperator
… properties in any way
…icMNState/CPro_Tx serialization
There was a problem hiding this 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
…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
…/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: 26f7e96 and 4b42dc8) 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
…/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: 26f7e96 and 4b42dc8) 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)_
…/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: 26f7e96 and 4b42dc8) 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)_
…/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: 26f7e96 and 4b42dc8) 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
Issue being fixed or feature implemented
same as #5392, alternative solution
based on #5402 atm, will rebase laterWhat 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: