-
Notifications
You must be signed in to change notification settings - Fork 78
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
native: implement HF-based update #3402
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3402 +/- ##
==========================================
- Coverage 86.12% 86.08% -0.05%
==========================================
Files 331 332 +1
Lines 38114 38339 +225
==========================================
+ Hits 32827 33005 +178
- Misses 3776 3804 +28
- Partials 1511 1530 +19 ☔ View full report in Codecov by Sentry. |
02adea3
to
a51a62d
Compare
@roman-khimov, the implementation seems to be in the final stage, I'm going to test it against C# node states for mainnet/testnet, and if everything is OK, then write a couple of unit tests. Need your opinion on the implementaiton structure, it differs from C# a bit, but behaviour is compatible. |
With the latest C# node fixes mainnet states match perfectly (with Cockatrace enabled at 5210001 height):
|
73abc17
to
07a218f
Compare
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.
Should be OK.
type MethodAndPrice struct { | ||
HFSpecificMethodAndPrice | ||
ActiveFrom *config.Hardfork |
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'm not sure why are you keeping all of them this way. You can have the one and only proper contract version at the current height and then just switch it when HF happens.
Historic calls maybe? These do require some dances.
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.
You can have the one and only proper contract version at the current height
That was an early version of this PR. And yes, due to the historic calls. To manage single contract version at a time, we need to initialize it every time when new historic call is processed (which I suppose is far from the best solution) or we need some kind of cache. So if we're to implement this cache anyway, then it's better to initialize it once during the node start in order not to affect the node performance during blocks processing. And, as discussed, we don't have a lot of natives/hardforks, so the cache size is not a problem for us (and we store pointers in cache anyway, as a result the most part of CS structures are shared between hardforks).
But that's just my thoughts. We can get back to one and only proper contract version
.
|
||
return s | ||
} | ||
|
||
// Initialize initializes Designation contract. It is called once at native Management's OnPersist | ||
// at the genesis block, and we can't properly fill the cache at this point, as there are no roles | ||
// data in the storage. | ||
func (s *Designate) Initialize(ic *interop.Context) error { | ||
func (s *Designate) Initialize(ic *interop.Context, hf *config.Hardfork) error { | ||
if hf != s.ActiveIn() { |
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.
Checking here...
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'm against of this change because I assume (from C# implementation and neo-project/neo#3200) that in future we still might want to (e.g.) put something into the contract storage on the next hardfork. And this code will be contract-dependent.
That's the whole point of neo-project/neo#3200, otherwise if we do call Initialize
only on deploy, then this issue is completely useless.
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.
What do you think?
storedJ, _ := json.Marshal(storedCS) | ||
autogenJ, _ := json.Marshal(autogenCS) | ||
return fmt.Errorf("native %s: version mismatch for the latest hardfork %s (stored contract state differs from autogenerated one), "+ | ||
"try to resynchronize the node from the genesis: %s vs %s", md.Name, current, string(storedJ), string(autogenJ)) |
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'm not sure it's a friendly error message. Many users may even not notice the real message in these JSON dumps.
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.
These dumps are not indented, so they are quite compact, I've tested it. I may add some indent. Or is it better to remove dumps at all?
|
||
// Hashes of all native contracts. | ||
var ( | ||
Management util.Uint160 |
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.
Just out of curiosity, can we have them precomputed automatically and saved as const
s? //go:generate
of some kind. We can combine them then with nativenames
probably, consts don't cost a lot.
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.
Check out the last commit, it's an interesting experiment, but I still want to keep Uint160
variables to reuse them in other code.
We can combine them then with nativenames probably,
They have different names, is it OK? E.g. nativenames.Gas
vs nativehashes.GasToken
. If it's OK, then I'll merge them into single package.
A part of #3213. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Ported as a part of neo-project/neo#2942. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
External users should use HF-specific methods and events. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Initialize all necessary HF-specific contract descriptors once during contract construction. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
With all associated native API changes ported from neo-project/neo#2925 and neo-project/neo#3154. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
It will give us a clue on what's wrong with contract states if something unexpected happen. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Similar to nativenames, instantiate once and then reuse everywhere. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
A part of #3213. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Ensure that Blockchain constructor is able to distinguish empty Hardforks map (no hardforks should be enabled) from nil hardforks map (the default value should be used in this case, i.e. all hardforks should be active from genesis). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
It's useful to keep the ordered set of native contract names. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
A part of #3213. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Ref. #3402 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@roman-khimov, do we need better unit-test coverage for this PR? |
Ref. #3402 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Ref. #3402 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Let our natives be updated! Close #3213.
TODO: