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
| @@ -0,0 +1,45 @@ | |||
| package contracts | |||
There was a problem hiding this comment.
I'd strip this directory, can be metachain.
| 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.
| metaContainersPrefix = iota | ||
| containerPlacementPrefix | ||
|
|
||
| // object prefixes. |
There was a problem hiding this comment.
Too many of them. Per-object meta can be stored in addrIndex, then other indexes should be added only if we care about specific lookup abilities (like pick all objects of type X or find if object has any children or is it locked, etc).
There was a problem hiding this comment.
so you mean a single kv addr->Serialize(metaMap) and some helper indexes, but NOT every header field we have, like for searching in metabase?
There was a problem hiding this comment.
Metabase is very much different because it has generic search capabilities. You don't have this requirement here, you can add indexes needed only for specific tasks. The main feature of this DB in fact is just an OID list. Sizes are nice as well (don't forget to calculate the sum). Lock status and parent-child relations.
| return fmt.Errorf("expected exactly 1 witness script, got %d", l) | ||
| } | ||
|
|
||
| _, pks, ok := scparser.ParseMultiSigContract(ic.Tx.Scripts[0].VerificationScript) |
There was a problem hiding this comment.
This bloats the transaction for no real reason. You have these keys in the contract, @AnnaShaleva can arrange a per-container verification script that would be short and have basically the same functionality (pack sigs and call some contract method to check).
| panic(fmt.Errorf("cannot retrieve placement vector from stack item: %w", err)) | ||
| } | ||
|
|
||
| err = isSignedBySNs(ic, placement) |
There was a problem hiding this comment.
With a correct verification script you could just check the signer account and this check better be done first.
There was a problem hiding this comment.
how do i know what signer to expect? it can be any node to sign, REP out of the full placement vector. i guess i still dont get you idea. is it about a really big and complex verification script?
There was a problem hiding this comment.
It's about small and easy verification script. Once you have CID you know it and you can hash it. #3759 (comment)
There was a problem hiding this comment.
i think i got what i had not understood, you suggest placing all the complex logic to CALL meta_check_sigs(cid, [][]sigs) call in the verification script, i didn't have such masterful skills before, i will try
| return fmt.Errorf("invalid verification script") | ||
| } | ||
|
|
||
| for i, vector := range placement { |
There was a problem hiding this comment.
And since you a single multisig you also have a problem parsing things here. Our placement policies are naturally a set of multisigs ("REP 3 + REP 2" is like "3 out of 9 + 2 out of 6"), so you should also naturally have a slice of input parameters here. Placement policies never change (even with nspcc-dev/neofs-api#352 the number of vectors is the same, only required number of signatures may change), so you can a verification script like
PUSH n_vectors
PACK
CALL meta_check_sigs(cid, [][]sigs)
And an invocation script of
PUSH sig1_1
PUSH sig1_2
PUSH sig1_3
PUSH 3
PACK
PUSH sig2_1
PUSH sig2_2
PUSH 2
PACK
// keep the order of arrays in mind
And there you have it --- two arrays of signatures in invocation script (not that convenient to sign, true, but all technically possible and not a problem once done), then an array of arrays created by verification script and passed into some contract method.
|
|
||
| // GAS represents GAS custom native contract. It always returns [DefaultBalance] as a | ||
| // balance, has no-op `Burn`, `Mint`, `Transfer` operations. | ||
| type GAS struct { |
There was a problem hiding this comment.
var _ = (native.IGAS)(&GAS{})| 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 <[email protected]>
Required for nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <[email protected]>
Required for initial version of Meta native contracts constructor, ref. nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <[email protected]>
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 <[email protected]>
Also, implement new MetaData contract and GAS contract implementation without economic. Signed-off-by: Pavel Karpy <[email protected]>
be48655 to
35c50ed
Compare
|
Fixed. |
Contract part of #3742.