Conversation
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) | ||
| } | ||
| } | ||
| return append(newContracts, meta.NewMetadata(neoContract)) |
There was a problem hiding this comment.
meta.NewMetadata does not seem resistant to nil arg. I'd precheck it
in general, looking at for-loop, we dont check absence of any contract. If they are required, i'd still prevent this
There was a problem hiding this comment.
added required contracts checks to NewCustomNatives
| newContracts = append(newContracts, contract) | ||
| case *native.Std, *native.Crypto, *native.Oracle, *native.Treasury: | ||
| default: | ||
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) |
There was a problem hiding this comment.
is this done to not miss new contract?
since native.NewDefaultContracts does not state there will be no more contract, i'd say this is an error
There was a problem hiding this comment.
A new contract is a significant (and very rare) event, this forces explicit handling of it. Should be ok.
There was a problem hiding this comment.
is this done to not miss new contract?
yes, *native.Treasury was even added after this feature was started to be implemented. my idea was that once there is an update in neo-go, tests will start panicking, and we will immediately find out whether we need to adopt a new contract to our side chain or not
| g := &GAS{ | ||
| initialSupply: init, | ||
| } | ||
| defer g.BuildHFSpecificMD(g.ActiveIn()) |
There was a problem hiding this comment.
defer seems redundant
There was a problem hiding this comment.
this is the way every native contract is implemented in neo-go, so i suggest to keep it
| return nil | ||
| } | ||
|
|
||
| // InitializeCache implements the Contract interface. |
There was a problem hiding this comment.
| // InitializeCache implements the Contract interface. | |
| // InitializeCache implements [native.IGAS] interface. |
for all methods
| } | ||
|
|
||
| // makeUint160Key creates a key from the account script hash. | ||
| func makeUint160Key(prefix byte, h util.Uint160) []byte { |
There was a problem hiding this comment.
unused too? Seems like some stuff is copy-pasted but not needed
| return fmt.Errorf("%d vector has incorrect REP: %w", i, err) | ||
| } | ||
| rep := repB.Int64() | ||
| if rep > maxREPsClauses { |
There was a problem hiding this comment.
i'd also check negatives
| Nodes keys.PublicKeys | ||
| } | ||
|
|
||
| func (p Placement) ToStackItem() (stackitem.Item, error) { |
There was a problem hiding this comment.
for some reason lost resolution for this thread: #3742 (comment)
is used now
| } | ||
| metaInfo, ok := metaInfoSI.Value().([]stackitem.MapElement) | ||
| if !ok { | ||
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T, given", metaInfo, metaInfoSI.Value())) |
There was a problem hiding this comment.
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T, given", metaInfo, metaInfoSI.Value())) | |
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T given", metaInfo, metaInfoSI.Value())) |
| } | ||
|
|
||
| if foundSigs == rep { | ||
| continue nodesLoop |
There was a problem hiding this comment.
instruction seems excessive here, condition may be inverted
There was a problem hiding this comment.
reimplementation of the byte-contract code from the neofs-contracts problems. fixed
| panic("incorrect vub") | ||
| } | ||
| if vub.Int64() <= int64(ic.BlockHeight()) { | ||
| panic("incorrect vub: exceeded") |
There was a problem hiding this comment.
runtime values may be helpful in exception
There was a problem hiding this comment.
added values everywhere a value is parsed and can be stringified
| newContracts = append(newContracts, contract) | ||
| case *native.Std, *native.Crypto, *native.Oracle, *native.Treasury: | ||
| default: | ||
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) |
There was a problem hiding this comment.
A new contract is a significant (and very rare) event, this forces explicit handling of it. Should be ok.
| ) | ||
|
|
||
| // nep17TokenNative represents a NEP-17 token contract. | ||
| type nep17TokenNative struct { |
There was a problem hiding this comment.
Wow. This should be imported from NeoGo native, really.
There was a problem hiding this comment.
really sorry, but it is not now
There was a problem hiding this comment.
This should be imported from NeoGo native
I don't think so. NeoGo's nep17TokenNative contains a lot of code that is specific to native Neo/Gas implementation and that is not needed for the simple meta GAS contract. I'd say that meta GAS doesn't even need nep17TokenNative, just define all methods on the GAS itself.
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("registerMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("unregisterMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
| panic("container does not support chained metadata") | ||
| } | ||
|
|
||
| sigsVectorsRaw, ok := args[1].Value().([]stackitem.Item) |
There was a problem hiding this comment.
This follows current container contract, but eventually it can be removed from parameters, nodes can sign transaction directly.
There was a problem hiding this comment.
this will require responding there with a signature not of an object's metadata, but with a signature of some predefined transaction. is that what you meant?
There was a problem hiding this comment.
Yes. Another option is a custom verification callback.
| } | ||
| } | ||
|
|
||
| // required |
There was a problem hiding this comment.
Why cid is checked separately?
There was a problem hiding this comment.
it is used in other checks above (container must exist and be registered as one that supports meta)
There was a problem hiding this comment.
I'd rather have similar checks at the same place.
|
|
||
| for _, sig := range sigVectors[i] { | ||
| for _, node := range placement[i].Nodes { | ||
| if node.Verify(sig, metaHash) { |
There was a problem hiding this comment.
i only suggest sorting signatures by public keys. requires support on the IR side and on the SN side
There was a problem hiding this comment.
It's not just about sorting. Remember, currently signatures are verified in verify() method of Proxy contract. This scales well. Checking them during transaction execution doesn't.
There was a problem hiding this comment.
But sorting is beneficial anyway and easily done.
There was a problem hiding this comment.
@roman-khimov, but what if number of nodes is bigger than 16? i found it occasionally when increased number of nodes in tests
do you mean that tx should be signed by nodes? or do you mean that we need Verify() method of the new native contract that should still get signatures from args and verify them (and, i guess, that new transactions should require the contract's signature to call Verify)?
There was a problem hiding this comment.
Multiple signatures for a single signer.
There was a problem hiding this comment.
@roman-khimov, did some multisignature magic, check, please, if i got you right
| } | ||
| if v, ok := getFromMap(metaInfo, "firstPart"); ok { | ||
| firstPart, err := v.TryBytes() | ||
| if err != nil || len(firstPart) != smartcontract.Hash256Len { |
There was a problem hiding this comment.
sha256 extraction is pretty popular
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
do you mean the secondary index must be in the same chain? my first implementation only needs side chain to check metadata and then notify about it. all the storage nodes handle notification on their own
There was a problem hiding this comment.
Among things current structure tries to solve is state synchronization. There is everything needed for it in blockchain, but if you're just throwing an event it all doesn't work. Primary index (based on submitted data) must be built directly on-chain. Then seconday one (header-based) is a different story.
There was a problem hiding this comment.
@roman-khimov, ok, adopted the previous metaservice approach. BUT currently there is oID+cID==64 problem for storage keys. i think we will eventually find a "one chain per container" solution, and it will be easier to work with keys, but currently i can only suggest skipping a single byte from oID
There was a problem hiding this comment.
Why do you care much about key size limits in native code?
bd68082 to
c8ae0ca
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3759 +/- ##
==========================================
- Coverage 25.63% 25.61% -0.02%
==========================================
Files 657 659 +2
Lines 42117 42149 +32
==========================================
+ Hits 10795 10797 +2
- Misses 30342 30373 +31
+ Partials 980 979 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c8ae0ca to
1f840ee
Compare
b7a798d to
13c1f32
Compare
978b2b0 to
4844514
Compare
roman-khimov
left a comment
There was a problem hiding this comment.
Better, but still can be optimized.
| g := &GAS{ | ||
| initialSupply: init, | ||
| } | ||
| defer g.BuildHFSpecificMD(g.ActiveIn()) |
| } | ||
|
|
||
| // NewGAS returns [GAS] custom native contract. | ||
| func NewGAS(init int64) *GAS { |
There was a problem hiding this comment.
You probably don't even need init argument because I suppose that initial supply won't be castomizable in meta chain, just use constant. You mock balances anyway, so the supply is not so important. I'd keep this mock GAS contract as simple as possible.
| _, totalSupply := g.getTotalSupply(ic.DAO) | ||
| if totalSupply.Sign() != 0 { | ||
| return errors.New("already initialized") | ||
| } | ||
| h, err := getStandbyValidatorsHash(ic) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| g.Mint(ic, h, big.NewInt(g.initialSupply), false) |
There was a problem hiding this comment.
Why do you ever need this code? You mock all the balances anyway.
@roman-khimov, is it intentionally?
There was a problem hiding this comment.
Not likely, can be omitted or can be not (balance < totalSupply is strange, but there are a lot of strange things here).
There was a problem hiding this comment.
this was a fast copy/paste move to have a working PoC. rechecked all the methods and dropped all the code that is never called
| ) | ||
|
|
||
| // nep17TokenNative represents a NEP-17 token contract. | ||
| type nep17TokenNative struct { |
There was a problem hiding this comment.
This should be imported from NeoGo native
I don't think so. NeoGo's nep17TokenNative contains a lot of code that is specific to native Neo/Gas implementation and that is not needed for the simple meta GAS contract. I'd say that meta GAS doesn't even need nep17TokenNative, just define all methods on the GAS itself.
| repB, err := vectorRaw[0].TryInteger() | ||
| if err != nil { | ||
| return fmt.Errorf("%d vector has incorrect REP: %w", i, err) | ||
| } | ||
| rep := repB.Int64() | ||
| if rep > maxREPsClauses { |
There was a problem hiding this comment.
stackitem.ToUint*, that's it.
| cID, err := requiredInMap(metaInfo, "cid").TryBytes() | ||
| if err != nil || len(cID) != smartcontract.Hash256Len { | ||
| panic("invalid container ID") | ||
| } |
There was a problem hiding this comment.
stackitem.ToUint256, reuse these helpers everywhere below.
| panic(fmt.Sprintf("incorrect object type: %s", err.Error())) | ||
| } | ||
| switch typ.Int64() { | ||
| case 0, 1, 2, 3, 4: // regular, tombstone, storage group, lock, link |
There was a problem hiding this comment.
for some reason, i forgot we have it in SDK, will fix
| metaInfoSI, err := stackitem.Deserialize(metaInfoRaw) | ||
| if err != nil { | ||
| panic(fmt.Errorf("cannot deserialize meta information from byte array: %w", err)) | ||
| } | ||
| metaInfo, ok := metaInfoSI.Value().([]stackitem.MapElement) | ||
| if !ok { | ||
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T given", metaInfo, metaInfoSI.Value())) | ||
| } |
There was a problem hiding this comment.
Is there any usual contract that interacts with serialized metaInfo map? If not, then we may change the serialization format to bytes or raw JSON (like it's done for native ContractManagement's NEF/Manifest arguments of deploy/update), no need to keep stackitem.Map.
There was a problem hiding this comment.
Native Neo format has lower overhead compared to JSON and I don't see any immediate benefits of JSON here (for manifests that's rather easy, there are a lot of tools that are interested in manifests).
| // - new native metadata contract (see [meta.NewMetadata] for details). | ||
| func NewCustomNatives(cfg neogoconfig.ProtocolConfiguration) []interop.Contract { | ||
| var ( | ||
| defaultContracts = native.NewDefaultContracts(cfg) |
There was a problem hiding this comment.
NACK. Don't do that, don't use native.NewDefaultContracts at all, it won't work correctly because the set of default native contracts uses cross-links to default native Neo/Gas/etc implementations. Instead, follow the https://github.com/nspcc-dev/neo-go/blob/fc4a77d3ead769e0c314c65fff53613718217278/pkg/core/custom_native_test.go#L216 example. Manually initialize those default native implementations that are required, initialize your custom contracts and properly set cross-links for all contracts.
There was a problem hiding this comment.
If you need some other default native implementations, then I'll just export them.
There was a problem hiding this comment.
it won't work correctly because the set of default native contracts uses cross-links
can you explain in details, please?
Manually initialize those default native implementations that are required, initialize your custom contracts and properly set cross-links for all contracts.
doesnt it look like calling NewDefaultContracts, dropping unused things, and adding new useful?
There was a problem hiding this comment.
can you explain in details, please?
For example, native Management uses INEO. Default native Management implementation created by native.NewDefaultContracts will use default native Neo implementation, whereas in your list of contracts you'll replace it with custom Neo. So you need to set internal links to native interfaces properly, like this:
https://github.com/nspcc-dev/neo-go/blob/fc52479b5b7c5bddd7b4b158308c04d72947d459/pkg/core/custom_native_test.go#L234
doesnt it look like calling NewDefaultContracts, dropping unused things, and adding new useful?
No, it's different. Follow the example: https://github.com/nspcc-dev/neo-go/blob/fc52479b5b7c5bddd7b4b158308c04d72947d459/pkg/core/custom_native_test.go#L216
|
|
||
| // NewCustomNatives returns custom list of native contracts for metadata | ||
| // side chain. Returned contracts: | ||
| // - NEO |
There was a problem hiding this comment.
@roman-khimov, we don't need default NeoContract for this chain. We don't need voting and rewards at all, is it true?
There was a problem hiding this comment.
i think, these lines require me to use smth, i tried to drop NEO contract. changing NEO to smth no-op can be done, i went the easiest way
There was a problem hiding this comment.
do you mean to fill it with smth different just to cut out voting and all?
There was a problem hiding this comment.
You need to implement custom Neo contract (like you did it with custom Gas). This contract should implement minimal required functionality for meta chain: committee tracking and committee witness checking, that's it (although I'm not sure whether we need dynamic committee at meta chain; let's start with static set of keys). The rest of INEO functionality should be stubbed. Voting should be dropped.
There was a problem hiding this comment.
You need to implement custom Neo contract
why i need it?
There was a problem hiding this comment.
Currently, we have decided to use regular Neo native since its excess functionality does not bother us for now, in the future, we may add a new minimal Neo contract
| // - NEO | ||
| // - Management | ||
| // - Ledger | ||
| // - Policy | ||
| // - Designate | ||
| // - Notary | ||
| // - redefined GAS (see [gas.NewGAS] for details) | ||
| // - new native metadata contract (see [meta.NewMetadata] for details). |
There was a problem hiding this comment.
Reorder comments in order to match the real order of custom natives. In general, the order of natives is important, it affects OnPersist/PostPersist behaviour.
Required for nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Required for nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Required for initial version of Meta native contracts constructor, ref. nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
00d5d10 to
6eee48f
Compare
|
Pushed smth wrong... |
6eee48f to
be48655
Compare
From neo-go side it is useful to have: * nspcc-dev/neo-go#4117, * nspcc-dev/neo-go#4145, * nspcc-dev/neo-go#4118. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Also, implement new MetaData contract and GAS contract implementation without economic. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
be48655 to
35c50ed
Compare
|
Fixed. |
roman-khimov
left a comment
There was a problem hiding this comment.
Specific indexes can be tuned, but signatures seem to be ok. Let's test it.
| md = native.NewMethodAndPrice(m.updateContainerList, 1<<15, callflag.WriteStates) | ||
| m.AddMethod(md, desc) | ||
|
|
||
| eDesc := native.NewEventDescriptor(putObjectEvent, |
| } | ||
|
|
||
| key[0] = addrIndex | ||
| deleteStorageItem(ic, key) |
|
|
||
| for i, vector := range placement { | ||
| var foundSigs, lastFoundSig int | ||
| for _, sig := range sigVectors[i] { |
There was a problem hiding this comment.
There is an optimized multisig checker in NeoGo, but likely not easily applicable.
Contract part of #3742.