container: add container lists to the contract #438

carpawell · 2024-10-02T07:50:35Z

It must be handled by the Alphabet and be updated after every epoch counter increase. Updating is done in two stage (filling a container list and commiting it) to prevent any stack size/memory restrictions. Closes #412.

contracts/container/contract.go

cthulhu-rider

lux gut overall

@carpawell will there be multiple Add's between Commit's? And might we need deletions?

cthulhu-rider · 2024-10-02T09:11:28Z

contracts/container/contract.go

+	common.CheckAlphabetWitness(multiaddr)
+
+	for _, node := range nodes {
+		pk := node[2 : 2+interop.PublicKeyCompressedLen] // public key in V2 marshaled node info


i'd follow 2636de0 and make consts. Moreover, given that this is a single protocol element, I would share the approach

depends on the final solution about what exact we store in the contract. will change if it is still proto NodeInfo

cthulhu-rider · 2024-10-02T09:15:24Z

contracts/container/contract.go

@@ -66,6 +67,8 @@ const (
 	containerKeyPrefix   = 'x'
 	ownerKeyPrefix       = 'o'
 	deletedKeyPrefix     = 'd'
+	nodesPrefix          = 'n'
+	nextEpochNodesPrefix = 'c'


couldnt the prefix match with estimateKeyPrefix cause potential collisions? i'd try to make prefixes as non-overlapping as possible

Yes, prefixes should be distinct. We do have some long ones for historic reasons, but new prefixes must not collide with them.

i tried to fit as much public key in key as possible (cid + pk is already more than contract can have as a key). are there any ideas how to store a list of smth that has 65-byte unique identifier in contract?

That's a separate question, here you can just change the prefix.

oh, this question only about the colision, ok, fixed

cthulhu-rider · 2024-10-02T09:31:40Z

contracts/container/contract.go

+// method must be called only when a container list is changed, otherwise
+// nothing should be done.
+// Call must be signed by the Alphabet nodes.
+func AddNextEpochNodes(cID []byte, nodes [][]byte) {


i think we should clarify what exactly these byte arrays are and verify them 1st. cID must be exactl 32B, each of nodes must be at least 2+interop.PublicKeyCompressedLen

Can we have just keys there? Everyone knows the network map anyway (can get details based on key). In fact, even a shorter address handle can be used, but this would require a bit more work (so keys are OK).

Also, please use more specific types. hash256 is much better than just []byte.

i think we should clarify what exactly these byte arrays are and verify them 1st

Also, please use more specific types. hash256 is much better than just []byte.

@roman-khimov, if it is hash256, can it be ensured by the VM? or still we need to check it manually?

Can we have just keys there?

we have discussed it a little and i thought that we make it clear that it is useless for current nodes and IR. if you need a container, then you need some ways to communicate with it, not just know its keys. it can be useful for the future things (#413), is this PR for contract-side calculations only?

if it is hash256, can it be ensured by the VM?

No, neo-project/neo#1891.

if you need a container, then you need some ways to communicate with it

Well, you can get a node by key. And full node info can take quite some space. But let's say it's OK for now.

No, neo-project/neo#1891.

fresh thing. ok, type is type, fixed

But let's say it's OK for now.

what exacty OK? this PR or storing keys only? i like storing only keys more. just want to be sure we all understand SN and IR will not be using it in the current code, only for new contract features. they will likely use a single network request, a single calculation (placement) and caching, not this PR

If we're targeting meta only then full node info isn't needed. But eventually I think this list can be used by other code since no one cares about placement policy, everyone just needs a node list to work with. Full node info can still be provided if this contract is to store keys only, not immediately, but after netmap refactorings. So overall keys only are OK.

cthulhu-rider · 2024-10-02T09:37:02Z

contracts/container/contract.go

+
+// CommitContainerListUpdate commits container list changes made by
+// [AddNextEpochNodes] calls in advance. If no [AddNextEpochNodes] have been
+// made, it clears container list. Mades "ContainerUpdate" notification with


makes or throws

cthulhu-rider · 2024-10-02T09:46:20Z

contracts/container/contract.go

+
+		storage.Delete(ctx, newNode.key[:])
+
+		newKey := append([]byte{nodesPrefix}, newNode.key[1:]...)


Suggested change

newKey := append([]byte{nodesPrefix}, newNode.key[1:]...)

newKey := append([]byte{nodesPrefix}, newNode.key[len(nextEpochNodesPrefix):]...)

or also make a const

not possible for now (it is a char literally, no length), likely be resolved automatically when we resolve other threads

cthulhu-rider · 2024-10-02T09:50:48Z

tests/container_test.go

+	stack, err := c.TestInvoke(t, "nodes", cID[:])
+	require.NoError(t, err)
+
+	iter, ok := stack.Pop().Value().(*storage.Iterator)


move this into iteratorToBytesArray and call it stackToByteArrays?

cthulhu-rider · 2024-10-02T10:05:02Z

contracts/container/contract.go

@@ -491,6 +494,67 @@ func List(owner []byte) [][]byte {
 	return list
 }

+// AddNextEpochNodes adds container members for the next epoch. Changes must be
+// committed with [CommitContainerListUpdate]. Without submitting, it is a no-op


i know what u mean, but no-op can cause misunderstaning: contract storage is changed, GAS spent, although user sees no side effect through the API. I'd say AddNextEpochNodes accumulates passed nodes as container members for the next epoch to be committed using [CommitContainerListUpdate].

i meant without submitting it is not possible to see any changes via public interfaces, it is an alphabet-only method, so gas and any other things are known and does not mean a lot. but don't mind your way

cthulhu-rider · 2024-10-02T10:10:17Z

tests/container_test.go

+	require.ElementsMatch(t, containerList, fromContract)
+
+	// clean up container
+	c.Invoke(t, stackitem.Null{}, "commitContainerListUpdate", cID[:])


1st i'd make one more addNextEpochNodes call with containerList subset + several completely new elements. And check that old elements are dropped. This is quite non-trivial with add-add-add... semantic sequence

contracts/container/config.yml

roman-khimov · 2024-10-02T14:33:17Z

contracts/container/contract.go

@@ -66,6 +67,8 @@ const (
 	containerKeyPrefix   = 'x'
 	ownerKeyPrefix       = 'o'
 	deletedKeyPrefix     = 'd'
+	nodesPrefix          = 'n'
+	nextEpochNodesPrefix = 'c'


Yes, prefixes should be distinct. We do have some long ones for historic reasons, but new prefixes must not collide with them.

roman-khimov · 2024-10-02T14:36:01Z

contracts/container/contract.go

+// method must be called only when a container list is changed, otherwise
+// nothing should be done.
+// Call must be signed by the Alphabet nodes.
+func AddNextEpochNodes(cID []byte, nodes [][]byte) {


Can we have just keys there? Everyone knows the network map anyway (can get details based on key). In fact, even a shorter address handle can be used, but this would require a bit more work (so keys are OK).

contracts/container/contract.go

roman-khimov · 2024-10-02T14:37:49Z

contracts/container/contract.go

+// method must be called only when a container list is changed, otherwise
+// nothing should be done.
+// Call must be signed by the Alphabet nodes.
+func AddNextEpochNodes(cID []byte, nodes [][]byte) {


Also, please use more specific types. hash256 is much better than just []byte.

contracts/container/contract.go

go.mod

contracts/container/config.yml

tests/container_test.go

carpawell · 2024-10-04T00:51:37Z

will there be multiple Add's between Commit's?

@cthulhu-rider, yes, if a list is too big we have to make it in more than one call, the stack is limited.

And might we need deletions?

You mean add-then-delete case? Will it be useful? What cases? New methods should be used by the alphabet nodes once per epoch if some container has changed, not sure I understand if an alphabet node calculates a container, puts it, and then decides to change it in a single epoch.

cthulhu-rider · 2024-10-04T04:54:02Z

You mean add-then-delete case? Will it be useful? What cases?

best answer is to ask questions huh :] delete question followed after multi-add one. I thought if there is "add" and no "set", where is "delete". Now i see that several "add" are essentially "set" so removals is not needed for now

cthulhu-rider

lets also update

neofs-contract/contracts/container/doc.go

Line 72 in 2059417

Contract storage model.

cthulhu-rider · 2024-10-04T05:31:28Z

contracts/container/contract.go

+	common.CheckAlphabetWitness(multiaddr)
+
+	counter := 0
+	counterKey := append([]byte{nextEpochNodesPrefix, 'i'}, cID...)


isnt this overcomplicated? We still have a good supply of prefixes. Collission, although unlikely, is possible

isnt this overcomplicated?

maybe. i did not want to have even more prefixes for the same feature, this is my suggestion. can add a dedicated counter prefix if @roman-khimov is also for it

Collission, although unlikely, is possible

to have problems we need either 31 of 32 bytes should match (with an offset) and the first one should be fixed i byte OR a container should have the first 31 bytes of i. it is very like "two different containers have the same ID" with almost the same kind of problems for NeoFS

You can avoid storing the counter if you're to take the last element of nextEpochNodesPrefix + cID which is easy with Backwards flag (https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop@v0.0.0-20241007161539-0968c3a81fb0/storage#FindFlags). But to do this you need to have counter serialized in BE form.

Or you can avoid using counter with https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop@v0.0.0-20241007161539-0968c3a81fb0/contract#CreateStandardAccount. This would solve duplicate problem at the same time.

@roman-khimov, idea with Backwards is the best for me, i like it

But to do this you need to have counter serialized in BE form.

how can i ensure it? are there any contract side helpers for it?

I don't think we have any, serialize+pad+reverse, maybe.

cthulhu-rider · 2024-10-04T05:41:57Z

contracts/container/contract.go

+		storage.Put(ctx, newKey, newNode.val)
+	}
+	if newCounter := storage.Get(ctx, newCounterKey); newCounter != nil {
+		storage.Put(ctx, oldCounterKey, storage.Get(ctx, newCounterKey))


why do we need to keep it?

and

Suggested change

storage.Put(ctx, oldCounterKey, storage.Get(ctx, newCounterKey))

storage.Put(ctx, oldCounterKey, newCounter)

not working?

why do we need to keep it?

technically no need, but there is a certain scheme: we have a counter and then this number of keys with counter suffixes. also can imagine requesting current number of container nodes, one Get may be simpler than searching for every entry
tbh, i did not even think about it, just moved all the "new" things to the "old" places. let it be dropped as not needed

cthulhu-rider · 2024-10-04T05:51:51Z

contracts/container/contract.go

+	if newCounter := storage.Get(ctx, newCounterKey); newCounter != nil {
+		storage.Put(ctx, oldCounterKey, storage.Get(ctx, newCounterKey))
+	} else {
+		storage.Delete(ctx, newCounterKey)


why if it is missing?

and shouldnt we always reset it?

dropped eventually

and shouldnt we always reset it?

but why not? meaningless counter, yes it may be overwritten with the next container update but before it happenes it is just a garbage

cthulhu-rider · 2024-10-04T05:56:01Z

contracts/container/contract.go

+// Call must be signed by the Alphabet nodes.
+func CommitContainerListUpdate(cID interop.Hash256) {
+	if len(cID) != interop.Hash256Len {
+		panic(cst.ErrorInvalidContainerID + std.Itoa10(len(cID)))


why differ with other methods?

fixed and added in test this call

carpawell · 2024-10-04T15:08:13Z

best answer is to ask questions huh :]

I cannot answer if i do not understand a question (even if it seems like a resolved one now, i still do not understand the original question and its resolution). You asked about deletion, i did not add this so i thought we did not need them. If you have some examples of its usage, i had to ask you to provide them, cause i did not see them. I hope this PR conveys my thoughts and my suggestions, otherwise, i need additional suggestions or change requests.

carpawell · 2024-10-04T15:08:32Z

lets also update

Hm, did not even know we have it. Updated.

roman-khimov · 2024-10-07T20:29:18Z

contracts/container/contract.go

+	common.CheckAlphabetWitness(multiaddr)
+
+	counter := 0
+	counterKey := append([]byte{nextEpochNodesPrefix, 'i'}, cID...)


You can avoid storing the counter if you're to take the last element of nextEpochNodesPrefix + cID which is easy with Backwards flag (https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop@v0.0.0-20241007161539-0968c3a81fb0/storage#FindFlags). But to do this you need to have counter serialized in BE form.

roman-khimov · 2024-10-07T20:32:06Z

contracts/container/contract.go

+	common.CheckAlphabetWitness(multiaddr)
+
+	counter := 0
+	counterKey := append([]byte{nextEpochNodesPrefix, 'i'}, cID...)


Or you can avoid using counter with https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop@v0.0.0-20241007161539-0968c3a81fb0/contract#CreateStandardAccount. This would solve duplicate problem at the same time.

contracts/container/contract.go

carpawell · 2024-10-11T18:04:42Z

@roman-khimov, recheck carefully, please, this is the 4th time i redo the PR, it may have stupid mistakes.

contracts/container/contract.go

It must be handled by the Alphabet and be updated after every epoch counter is increased. Updating is done in two stage (filling a container list and commiting it) to prevent any stack size/memory restrictions. Closes #412. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

After nspcc-dev/neofs-contract#438 Container contract now has API for container-side placement/verification operations. But building placement vectors and updating when needed is still a complex operation that should be done on a Go application side. This commit adds such a responsibility for Alphabet nodes. For simplicity, updating is done every epoch and once for _every_ container if netmap has been changed. Additional optimization can be considered. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

After nspcc-dev/neofs-contract#438 Container contract now has API for container-side placement/verification operations. But building placement vectors and updating when needed is still a complex operation that should be done on a Go application side. This commit adds such a responsibility for Alphabet nodes. For simplicity, updating is done every epoch and once for _every_ container if netmap has been changed. Additional optimization can be considered. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

After nspcc-dev/neofs-contract#438 Container contract now has API for container-side placement/verification operations. But building placement vectors and updating when needed is still a complex operation that should be done on a Go application side. This commit adds such a responsibility for Alphabet nodes. For simplicity, updating is done every epoch and once for _every_ container if netmap has been changed. Additional optimization can be considered. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

After nspcc-dev/neofs-contract#438 Container contract now has API for container-side placement/verification operations. But building placement vectors and updating when needed is still a complex operation that should be done on a Go application side. This commit adds such a responsibility for Alphabet nodes. For simplicity, updating is done every epoch and once for _every_ container if netmap has been changed. Additional optimization can be considered. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

After nspcc-dev/neofs-contract#438 Container contract now has API for container-side placement/verification operations. But building placement vectors and updating when needed is still a complex operation that should be done on a Go application side. This commit adds such a responsibility for Alphabet nodes. For simplicity, updating is done every epoch and once for _every_ container if netmap has been changed. Additional optimization can be considered. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell requested a review from AnnaShaleva October 2, 2024 07:50

carpawell self-assigned this Oct 2, 2024

carpawell requested review from cthulhu-rider and roman-khimov as code owners October 2, 2024 07:50

carpawell force-pushed the feat/add-container-list branch from 316ab70 to c68c652 Compare October 2, 2024 07:54

carpawell commented Oct 2, 2024

View reviewed changes

contracts/container/contract.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/add-container-list branch from c68c652 to 2ae28ee Compare October 2, 2024 07:59

cthulhu-rider reviewed Oct 2, 2024

View reviewed changes

roman-khimov reviewed Oct 2, 2024

View reviewed changes

carpawell force-pushed the feat/add-container-list branch from 2ae28ee to a52d1eb Compare October 4, 2024 00:38

carpawell requested review from cthulhu-rider and roman-khimov October 4, 2024 00:54

cthulhu-rider reviewed Oct 4, 2024

View reviewed changes

carpawell force-pushed the feat/add-container-list branch from a52d1eb to 948f97b Compare October 4, 2024 15:00

carpawell requested a review from cthulhu-rider October 4, 2024 15:09

roman-khimov reviewed Oct 7, 2024

View reviewed changes

carpawell force-pushed the feat/add-container-list branch from 948f97b to d1675a2 Compare October 10, 2024 03:12

carpawell requested a review from roman-khimov October 10, 2024 03:28

carpawell mentioned this pull request Oct 10, 2024

Limit max REP value and number of REPs nspcc-dev/neofs-api#307

Closed

carpawell force-pushed the feat/add-container-list branch 2 times, most recently from e9c8966 to d344274 Compare October 11, 2024 03:37

This was referenced Oct 11, 2024

Provide library functions to convert numbers into BE-ordered byte strings nspcc-dev/neo-go#3606

Open

Provide KV type in the interop/storage package nspcc-dev/neo-go#3607

Open

roman-khimov reviewed Oct 11, 2024

View reviewed changes

This was referenced Oct 11, 2024

Iterating over nil (null) slices lead to an error nspcc-dev/neo-go#3608

Open

Item cast to struct does not ensure/convert field type nspcc-dev/neo-go#3610

Open

carpawell mentioned this pull request Oct 11, 2024

Array's items swap compiles but does not work as expected nspcc-dev/neo-go#3611

Open

carpawell force-pushed the feat/add-container-list branch 3 times, most recently from 62b22e9 to f0f04ad Compare October 11, 2024 18:03

carpawell requested a review from roman-khimov October 11, 2024 18:03

carpawell force-pushed the feat/add-container-list branch from f0f04ad to 7b231b8 Compare October 11, 2024 18:10

roman-khimov reviewed Oct 12, 2024

View reviewed changes

contracts/container/contract.go Outdated Show resolved Hide resolved

contracts/container/contract.go Outdated Show resolved Hide resolved

contracts/container/contract.go Outdated Show resolved Hide resolved

contracts/container/contract.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/add-container-list branch from 7b231b8 to b7d6207 Compare October 13, 2024 17:37

carpawell force-pushed the feat/add-container-list branch from b7d6207 to 660756f Compare October 13, 2024 17:39

carpawell requested a review from roman-khimov October 13, 2024 17:39

roman-khimov approved these changes Oct 14, 2024

View reviewed changes

roman-khimov merged commit 6a5813a into master Oct 14, 2024
8 checks passed

roman-khimov deleted the feat/add-container-list branch October 14, 2024 07:19

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 10, 2024

morph: add method for container's members updates

e987e2e

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 11, 2024

morph: add method for container's members updates

f36e9ca

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 11, 2024

morph: add method for container's members updates

2523906

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 12, 2024

morph: add method for container's members updates

75eaea8

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 23, 2024

morph: add method for container's members updates

59cebb3

Follows nspcc-dev/neofs-contract#438. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>


		storage.Delete(ctx, newNode.key[:])

		newKey := append([]byte{nodesPrefix}, newNode.key[1:]...)

	storage.Put(ctx, oldCounterKey, storage.Get(ctx, newCounterKey))
	storage.Put(ctx, oldCounterKey, newCounter)

container: add container lists to the contract #438

container: add container lists to the contract #438

Conversation

carpawell commented Oct 2, 2024

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Oct 4, 2024

cthulhu-rider commented Oct 4, 2024

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Oct 4, 2024

carpawell commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Oct 11, 2024

cthulhu-rider Oct 2, 2024 •

edited

Loading

carpawell Oct 4, 2024 •

edited

Loading