forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: model interface after MnNetInfo and support switching impls, add new ProTx version
#6665
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c4f1175
evo: cleanup CDeterministicMN{,List,ListDiff,StateDiff} ser logic
kwvg 0a4e726
util: add deep comparison helpers for shared_ptr
kwvg 0b04978
evo: make `netInfo` a shared pointer
kwvg f212caa
evo: introduce `NetInfoInterface`, make `MnNetInfo` an implementation
kwvg 22d9e93
evo: use interface shared ptr for `netInfo` instead of implementation
kwvg c7948e6
evo: allow deciding `NetInfoInterface` impl based on object version
kwvg b4fa7a2
evo: add helper to get highest ProTx version based on deployment status
kwvg 3e65628
evo: introduce new ProTx version for extended addresses (`ExtNetInfo`)
kwvg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hmm.. What happens when there are more new versions?.. should we allow them here? i.e.
Also, don't we need a similar thing for the new
newState->nVersion = opt_proTx->nVersion;inTRANSACTION_PROVIDER_UPDATE_SERVICEpart above?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.
If there is a version after
ExtAddr, this code will need to be revisited, the change here is meant to ensure two things:LegacyBLStoBasicBLStransition is honored (usingopt_proTx->nVersionfrom theProUpRegTx)ExtAddr(either from aProRegTxor aProUpServTx), theProUpRegTx(which can only have a version up toBasicBLS), doesn't downgradenewState->nVersion(set asExtAddr) toBasicBLS(fromopt_proTx->nVersion) by settingnewState->nVersionto itself.This handling isn't needed for
TRANSACTION_PROVIDER_UPDATE_SERVICEsince we want to updatenVersionto reflectExtAddr.This is alongside a two-pronged check
ProUpRegTxwithnVersion = ExtAddr(by refusing to check the deployment status of extended addresses if not aProRegTxor aProUpServTx, forcing the highest valid version to beBasicBLS) to prevent a potentialLegacyBLStoExtAddrleap (source)ProUpServTxdoesn't cause a potentialLegacyBLStoExtAddrleap (source)Uh oh!
There was an error while loading. Please reload this page.
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.
Thinking more about this issue - should we even care? These txes can be created by MN owners/operators only and only for the MN they own/operate, so no one else is affected. Maybe we should simply let them mess it up for now if they decide to use some custom code (instead of our RPCs) to create these txes? New protx upgrade/downgrade checks (which are activated with v23) will handle that later in a proper place.
Uh oh!
There was an error while loading. Please reload this page.
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.
I am not comfortable with leaving the masternode state in an inconsistent state, since that affects all clients that could process the aberrant transaction. I would be more amicable to strip down some of the checks if the worst that could happen is corrupting the local data directory but the wider implications mean that it's better to impose greater restrictions.
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.
Actually, this condition will only work for post-v23 txes (
newState->nVersion > ProTxVersion::BasicBLS) and you have checks for them inCheckProUpRegTx()which are activated with v23 so this extra logic here is simply redundant probably.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.
@UdjinM6 is this resolvable?
Uh oh!
There was an error while loading. Please reload this page.
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.
my suggestion is to keep it
newState->nVersion = opt_proTx->nVersion;here + 5c9a116There 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.
ProUpRegTxcannot be versionExtAddranyways, it is capped atBasicBLS, the change is to ensure that there is a registrar update (which naturally can only be up toBasicBLS), it doesn't apply its transaction version to the state if it's a lower version thanExtAddr, which is set byProUpServTx.To prevent a future relaxation of maximum version checks from setting
ProUpRegTxto versionExtAddr, a more explicit check has been added in dash#6729 (92984c9)Uh oh!
There was an error while loading. Please reload this page.
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.
That's the part I don't like
But what if there is another version upgrade in the future which should be done via
ProUpRegTxtoo?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.
It's not great but I briefly tried uniformly setting the nVersion to
ExtAddrbut it seemed troubling, even if we can constrain application of versioning on the internal state, the version bump indicates no actual change in the transaction itself, requiring selective ignorance of the version field.Creates this weird situation where the version change (which indicates a change in serialization) may either hint at a change or none at all. This wasn't really a problem in the past since the BLS version update affected all transactions but the way the dmnState handles things, it never envisioned changes to only some transaction types but not others.
So at that point, dmnState needs to track what the effective serialization would be based on the highest version set among all transactions, which means monitoring for unintended version backsliding.
Then the code can be modified again to be