feat/Make `NewEpoch` callback mechanism more generic #368

carpawell · 2023-11-20T17:10:49Z

Do not hardcode container and balance contract as a NewEpoch events receivers in the netmap contract. Use the netmap's registration method on a deploy stage instead.

roman-khimov

Tests fail.

netmap/netmap_contract.go

netmap/config.yml

balance/balance_contract.go

common/netmap.go

container/container_contract.go

netmap/netmap_contract.go

common/netmap.go

container/config.yml

netmap/netmap_contract.go

carpawell · 2023-11-23T17:08:24Z

Ready one more time. Look carefully, please.

netmap/netmap_contract.go

cthulhu-rider · 2023-11-24T07:38:56Z

common/netmap.go

+// callback requester. Netmap contract's address is taken from the
+// NNS contract, therefore, it must be presented and filled with
+// netmap information for a correct SubscribeForNewEpoch call; otherwise
+// a successive call is not guaranteed.


what call is this line talking about? not guaranteed seems like some kind of random

what call is this line talking about?

method call

not guaranteed seems like some kind of random

i meant it depends on an NNS implementation. it may and may not panic

i've read this doc several times and not sure i completely understand it. We impose some preconditions (Netmap contract registered in NNS). If complied with - success, otherwise panic, right? Where is not guaranteed come from?

filled with netmap information - who must be filled and what information?

If complied with - success, otherwise panic, right?

well, to me that is not for sure. you need this to work it OK, but if it is not true, it depends on the NNS contract implementation. this call maybe a part of transaction, and panic (exception) may fail the whole TX

Where is not guaranteed come from?

that is how i said "undefined behavior" but that time i wanted to say it this way

filled with netmap information - who must be filled and what information?

NNS contract

cthulhu-rider · 2023-11-24T07:40:49Z

common/netmap.go

+// NNS contract, therefore, it must be presented and filled with
+// netmap information for a correct SubscribeForNewEpoch call; otherwise
+// a successive call is not guaranteed.
+// Caller must have `NewEpoch` method with a single numeric argument.


possible to check?

https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop/native/management#HasMethod

@roman-khimov, no args' types check?

It doesn't care even for actual calls (hi, neo-project/neo#1891) and you want it to check a complete signature in hasMethod...

cthulhu-rider · 2023-11-24T07:44:20Z

common/nns.go

+	"github.com/nspcc-dev/neo-go/pkg/interop/native/std"
+)
+
+// ResolveContractHash resolves contract hash by its well-known NeoFS name.


well known by whom?

Suggested change

// ResolveContractHash resolves contract hash by its well-known NeoFS name.

// ResolveContractHash hash of the NeoFS contract by its NNS domain name.

btw is there any place where we doc these names?

ResolveContractHash hash of the NeoFS contract

what do you want to say by it?

by its NNS domain name.

not sure we can say it. that is not the full NNS name, a caller should pass just container, not container.neofs

cthulhu-rider · 2023-11-24T07:46:08Z

common/nns.go

+)
+
+// ResolveContractHash resolves contract hash by its well-known NeoFS name.
+// Contract name should be lowercased, should not include `.neofs` TLD. Example


its better to either require name from the names list (rel thread above) or just should be a valid NNS domain

its better to either require name from the names list (rel thread above)

the list can grow and docs arent updated usually

should be a valid NNS domain

but container is not a valid NNS name

cthulhu-rider · 2023-11-24T07:48:22Z

common/nns.go

+// values: "netmap", "container", etc.
+// Relies on some NeoFS specifics:
+//  1. NNS contract should be deployed first (and should have `1` contract ID)
+//  2. It should be prefilled with contract hashes by their names (no


isnt this obvious for ResolveContractHash semantics? If it's not prefilled -> resolving fails

NNS is kinda general thing and we are not the only ones who have it. so i wanted to highlight what exactly should be done: NNS is deployed and it should map string -> uint160 (although i understand that the only callers of this method is are our contracts)

cthulhu-rider · 2023-11-24T08:20:02Z

netmap/netmap_contract.go

@@ -73,7 +73,8 @@ const (
 	containerContractKey = "containerScriptHash"
 	balanceContractKey   = "balanceScriptHash"

-	cleanupEpochMethod = "newEpoch"
+	newEpochSubscribersKey = "newEpochSubscribers"


Suggested change

newEpochSubscribersKey = "newEpochSubscribers"

newEpochSubscribersKeyPrefix = "newEpochSubscribers"

Please use one byte prefixes, they don't have to be this long and we can only have 64-byte keys.

newEpochSubscribersKeyPrefix

ok but without key then

Please use one byte prefixes

n won my democratic election

cthulhu-rider · 2023-11-24T08:22:58Z

netmap/migration_test.go

+	// contracts are migrated in alphabetical order at least for now
+
+	contract.SeekStorage(append(newEpochSubsNewPrefix, 0), func(k, v []byte) bool {
+		balanceGot, err := util.Uint160DecodeBytesLE(k)


u dont check func entrance. I'd gather map index->address and check after SeekStorage call

fixed by bool flags

cthulhu-rider · 2023-11-24T08:24:41Z

netmap/netmap_contract.go

@@ -601,6 +605,32 @@ func ListConfig() []record {
 	return config
 }

+// SubscribeForNewEpoch registers passed contract as a NewEpoch event
+// subscriber. Such a contract must have a `NewEpoch` method with a single
+// numeric argument. Transactions that call SubscribeForNewEpoch must be


Suggested change

// numeric argument. Transactions that call SubscribeForNewEpoch must be

// numeric parameter. Transactions that call SubscribeForNewEpoch must be

cthulhu-rider · 2023-11-24T08:25:11Z

netmap/netmap_contract.go

+// subscriber. Such a contract must have a `NewEpoch` method with a single
+// numeric argument. Transactions that call SubscribeForNewEpoch must be
+// witnessed by the Alphabet.
+// Produces `NewEpochHookRegistration` notification event with a just


according to code it is

Suggested change

// Produces `NewEpochHookRegistration` notification event with a just

// Produces `NewEpochSubscription` notification event with a just

fixed. it did not live renamings well

cthulhu-rider · 2023-11-24T08:26:44Z

tests/migration/storage.go

@@ -252,6 +252,13 @@ func (x *Contract) GetStorageItem(key []byte) []byte {
 	return x.exec.Chain.GetStorageItem(x.id, key)
 }

+// SeekStorage calls a provided handler against every stored item that starts
+// with a provided prefix. On handler's `false` return stops iteration. prefix
+// is removed from the resulting pair.


Suggested change

// is removed from the resulting pair.

// is removed from the resulting pair key.

?

cthulhu-rider · 2023-11-24T08:40:19Z

1st commit message has strange quotes

roman-khimov · 2023-11-24T08:52:30Z

common/netmap.go

+// NNS contract, therefore, it must be presented and filled with
+// netmap information for a correct SubscribeForNewEpoch call; otherwise
+// a successive call is not guaranteed.
+// Caller must have `NewEpoch` method with a single numeric argument.


https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/interop/native/management#HasMethod

common/netmap.go

roman-khimov · 2023-11-24T09:00:32Z

netmap/netmap_contract.go

@@ -73,7 +73,8 @@ const (
 	containerContractKey = "containerScriptHash"
 	balanceContractKey   = "balanceScriptHash"

-	cleanupEpochMethod = "newEpoch"
+	newEpochSubscribersKey = "newEpochSubscribers"


Please use one byte prefixes, they don't have to be this long and we can only have 64-byte keys.

netmap/netmap_contract.go

carpawell · 2023-11-24T18:33:25Z

1st commit message has strange quotes

fixed

common/netmap.go

roman-khimov · 2023-11-26T11:00:43Z

common/netmap.go

@@ -12,6 +14,11 @@ import (
 // a successive call is not guaranteed.
 // Caller must have `NewEpoch` method with a single numeric argument.
 func SubscribeForNewEpoch() {
+	callingHash := runtime.GetExecutingScriptHash()


It's executing script hash and you're calling it calling.

i meant that is a contract that called that method. renamed to callingContract, it is ok?

roman-khimov · 2023-11-26T11:02:11Z

common/netmap.go

@@ -12,6 +14,11 @@ import (
 // a successive call is not guaranteed.
 // Caller must have `NewEpoch` method with a single numeric argument.
 func SubscribeForNewEpoch() {
+	callingHash := runtime.GetExecutingScriptHash()
+	if !management.HasMethod(callingHash, "newEpoch", 1) {


Why this check is here and not in the subscribeForNewEpoch itself? It makes much more sense there since netmap controls its state and shouldn't allow bad subscriptions (forget contract upgrades for a moment).

moved to the netmap contract

netmap/netmap_contract.go

AnnaShaleva

LGTM otherwise, but these are minor comments and may be left unresolved.

AnnaShaleva · 2023-11-27T16:04:37Z

netmap/netmap_contract.go

+	it := storage.Find(ctx, newEpochSubscribersPrefix, storage.RemovePrefix|storage.KeysOnly)
+	for iterator.Next(it) {
+		contractHash := interop.Hash160(iterator.Value(it).([]byte)[1:]) // one byte is for number prefix
+		contract.Call(contractHash, cleanupEpochMethod, contract.All, epoch)


And one more important moment that just came to my mind. If something (panic, I mean) happens in the calling callback, then the rest of callbacks won't be executed. It remains PostPersist and technically it's OK to fail immediately, so just a question: do we need to think about some exception handlers here?

do we need to think about some exception handlers here?

only our own contracts are expected to be new-epoch-notified so any panic is a global panic to me and i can not say that some contract is more or less important to us, we need all of them to be notified. failed notifications are a failed NeoFS network to me. also, i do know what such a handler should do. notify, panic but later, log?

@roman-khimov

Partial failure in many ways is better than complete failure, so handling exceptions makes some sense here, but I'd leave it to another issue.

AnnaShaleva · 2023-11-27T16:09:12Z

netmap/netmap_contract.go

+		raw := iterator.Value(it).([]byte)[1:] // 1 byte is an index
+		if contract.Equals(raw) {
+			return
+		}


I'm a paranoiac about VM's EQUAL and compiler, could we have a test for the contract that subscribes for notifications and after that subscribes one more time? In the end we should check that there's only one subscriber record stored in the netmap.

AnnaShaleva · 2023-11-27T16:15:47Z

netmap/netmap_contract.go

+		addrBalance    interop.Hash160 // Balance contract not used legacy
+		addrContainer  interop.Hash160 // Container contract not used legacy


BTW, if these two fields are unused, then you can explicitly mark them as _.

Remove hardcoded "container" as a nice name TLD. It looks like a bug and has nothing useful. Also, fix tests that register "neofs" as a nice name TLD, that does not allow any contract test changes with the common TLD ("neofs") registration. Signed-off-by: Pavel Karpy <[email protected]>

Do not hardcode container and balance contract as a `NewEpoch` events receivers in the netmap contract. Use the new netmap's registration method on subscribers' deployment stages instead. Closes #327. Signed-off-by: Pavel Karpy <[email protected]>

Signed-off-by: Pavel Karpy <[email protected]>

Subscribe only contracts that can handle it with a corresponding method. Method's arg type is currently not possible to check but that is a Neo story not that repo's or the commit's one. Signed-off-by: Pavel Karpy <[email protected]>

Signed-off-by: Pavel Karpy <[email protected]>

With `nolint` of course. Seems like `unused` ignores funcs that are marked with `nolint` and does not see the old keys usage. Signed-off-by: Pavel Karpy <[email protected]>

carpawell · 2023-11-27T17:33:25Z

Some var renamings, commit body fixes, sry.

carpawell self-assigned this Nov 20, 2023

carpawell requested review from cthulhu-rider and roman-khimov as code owners November 20, 2023 17:10

carpawell force-pushed the feat/new-epoch-hook branch from dc252aa to 7a601b0 Compare November 20, 2023 17:12

carpawell requested a review from AnnaShaleva November 20, 2023 17:12

roman-khimov requested changes Nov 20, 2023

View reviewed changes

AnnaShaleva reviewed Nov 21, 2023

View reviewed changes

common/netmap.go Outdated Show resolved Hide resolved

common/netmap.go Outdated Show resolved Hide resolved

container/config.yml Outdated Show resolved Hide resolved

netmap/netmap_contract.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/new-epoch-hook branch from 7a601b0 to 2bebea7 Compare November 22, 2023 21:19

carpawell marked this pull request as draft November 22, 2023 21:19

carpawell force-pushed the feat/new-epoch-hook branch from 2bebea7 to 0623894 Compare November 23, 2023 17:07

carpawell marked this pull request as ready for review November 23, 2023 17:08

carpawell commented Nov 23, 2023

View reviewed changes

netmap/netmap_contract.go Show resolved Hide resolved

carpawell requested review from roman-khimov and AnnaShaleva November 23, 2023 17:09

AnnaShaleva reviewed Nov 23, 2023

View reviewed changes

netmap/netmap_contract.go Show resolved Hide resolved

netmap/netmap_contract.go Outdated Show resolved Hide resolved

netmap/netmap_contract.go Show resolved Hide resolved

netmap/netmap_contract.go Show resolved Hide resolved

netmap/netmap_contract.go Show resolved Hide resolved

carpawell force-pushed the feat/new-epoch-hook branch from 0623894 to 3cbdba1 Compare November 23, 2023 17:50

cthulhu-rider reviewed Nov 24, 2023

View reviewed changes

roman-khimov reviewed Nov 24, 2023

View reviewed changes

carpawell force-pushed the feat/new-epoch-hook branch from 3cbdba1 to b016c50 Compare November 24, 2023 14:28

carpawell force-pushed the feat/new-epoch-hook branch from b016c50 to 2957a7e Compare November 24, 2023 18:33

roman-khimov reviewed Nov 24, 2023

View reviewed changes

common/netmap.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/new-epoch-hook branch from 2957a7e to 0f80410 Compare November 25, 2023 11:34

carpawell requested review from roman-khimov, cthulhu-rider and AnnaShaleva November 25, 2023 11:35

roman-khimov reviewed Nov 26, 2023

View reviewed changes

carpawell force-pushed the feat/new-epoch-hook branch from 0f80410 to 2d8df56 Compare November 27, 2023 08:51

carpawell requested review from roman-khimov and AnnaShaleva November 27, 2023 11:08

roman-khimov reviewed Nov 27, 2023

View reviewed changes

netmap/netmap_contract.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/new-epoch-hook branch from b34ad43 to c7dae34 Compare November 27, 2023 12:08

carpawell requested a review from roman-khimov November 27, 2023 12:09

roman-khimov previously approved these changes Nov 27, 2023

View reviewed changes

AnnaShaleva previously approved these changes Nov 27, 2023

View reviewed changes

AnnaShaleva reviewed Nov 27, 2023

View reviewed changes

AnnaShaleva mentioned this pull request Nov 27, 2023

Drop most of deploy configs #370

Closed

carpawell dismissed stale reviews from AnnaShaleva and roman-khimov via 8b11124 November 27, 2023 17:19

carpawell force-pushed the feat/new-epoch-hook branch 2 times, most recently from 8b11124 to 889c64c Compare November 27, 2023 17:22

carpawell requested a review from AnnaShaleva November 27, 2023 17:22

carpawell force-pushed the feat/new-epoch-hook branch from 889c64c to a52b3fe Compare November 27, 2023 17:24

roman-khimov previously approved these changes Nov 27, 2023

View reviewed changes

carpawell added 4 commits November 27, 2023 20:30

common: DRY resolving hashes via NNS contract

b4bd9b6

Signed-off-by: Pavel Karpy <[email protected]>

common: Filter newEpoch subscribers

57acf79

Subscribe only contracts that can handle it with a corresponding method. Method's arg type is currently not possible to check but that is a Neo story not that repo's or the commit's one. Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the feat/new-epoch-hook branch from a52b3fe to 3fc8070 Compare November 27, 2023 17:30

carpawell added 2 commits November 27, 2023 20:32

netmap: Do not allow double new epoch subscribing

5e5c25f

Signed-off-by: Pavel Karpy <[email protected]>

netmap: Fix unused linter

34f642f

With `nolint` of course. Seems like `unused` ignores funcs that are marked with `nolint` and does not see the old keys usage. Signed-off-by: Pavel Karpy <[email protected]>

carpawell dismissed roman-khimov’s stale review via 34f642f November 27, 2023 17:32

carpawell force-pushed the feat/new-epoch-hook branch from 3fc8070 to 34f642f Compare November 27, 2023 17:32

carpawell requested a review from roman-khimov November 27, 2023 17:33

roman-khimov approved these changes Nov 27, 2023

View reviewed changes

roman-khimov merged commit a72a205 into master Nov 27, 2023
8 checks passed

roman-khimov deleted the feat/new-epoch-hook branch November 27, 2023 19:10

	// ResolveContractHash resolves contract hash by its well-known NeoFS name.
	// ResolveContractHash hash of the NeoFS contract by its NNS domain name.

	newEpochSubscribersKey = "newEpochSubscribers"
	newEpochSubscribersKeyPrefix = "newEpochSubscribers"

	// numeric argument. Transactions that call SubscribeForNewEpoch must be
	// numeric parameter. Transactions that call SubscribeForNewEpoch must be

	// Produces `NewEpochHookRegistration` notification event with a just
	// Produces `NewEpochSubscription` notification event with a just

	// is removed from the resulting pair.
	// is removed from the resulting pair key.

		addrBalance interop.Hash160 // Balance contract not used legacy
		addrContainer interop.Hash160 // Container contract not used legacy

feat/Make NewEpoch callback mechanism more generic #368

feat/Make NewEpoch callback mechanism more generic #368

Conversation

carpawell commented Nov 20, 2023

roman-khimov left a comment

Choose a reason for hiding this comment

carpawell commented Nov 23, 2023

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

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 commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Nov 24, 2023

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

AnnaShaleva 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

carpawell commented Nov 27, 2023

feat/Make `NewEpoch` callback mechanism more generic #368

feat/Make `NewEpoch` callback mechanism more generic #368