-
Notifications
You must be signed in to change notification settings - Fork 68
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
Clean-up activation heights #1094
base: release/1.4.0.0
Are you sure you want to change the base?
Clean-up activation heights #1094
Conversation
@@ -107,8 +107,8 @@ public CirrusDev() | |||
var bip9Deployments = new CirrusBIP9Deployments() | |||
{ | |||
// Deployment will go active once 75% of nodes are on 1.3.0.0 or later. | |||
[CirrusBIP9Deployments.Release1320] = new BIP9DeploymentsParameters("Release1320", CirrusBIP9Deployments.FlagBitRelease1320, DateTime.Parse("2022-6-15 +0").ToUnixTimestamp() /* Activation date lower bound */, DateTime.Now.AddDays(50).ToUnixTimestamp(), BIP9DeploymentsParameters.DefaultRegTestThreshold), |
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 see that there is no activation heights array defined for this network, is that intentional?
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.
Yes, it intentionally defaults the activation height to 0 by omitting the array initialization.
@@ -91,10 +111,11 @@ public class PoAConsensusOptions : ConsensusOptions | |||
public int PollExpiryBlocks { get; set; } | |||
|
|||
/// <summary> | |||
/// Defines when V2 of the contract serializer will be used. | |||
/// I.e if tip <= ContractSerializerV2ActivationHeight, V1 will be used. | |||
/// Leaving this for now for use by Stratis.SmartContracts.CLR. |
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.
Is this comment still applicable? It seems that ContractPrimitiveSerializer
is using the new approach
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.
The idea is to reduce the chances of possible version incompatibilities where projects may still be using the older Stratis.SmartContracs.CLR
NuGets such as Stratis.SmartContracts
within say test projects that use network classes that don't have the field available. Can't remember the exact circumstance that caused issues for me but I concluded this would do more good than harm and can easily be removed in the future.
This approach looks sound, and I don't see any problems with the PR per se. However, this functionality is effectively mimicing what the |
The challenge is to have a different set of index constants for PoA seeing a specific |
Made the necessary changes to be able to derive |
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.
Very nice, we just have to be careful with the implicit setting of the buried deployment heights to 0
on multiple networks when the enum is extended
This PR:
PoABuriedDeploymentsArray
. Some advantages to this:PoAConsensusOptions
class-definition will remain static instead of changing with almost every release.