policy: Fix IteratorChainsByPrefix #84

aarifullin · 2024-04-03T13:56:25Z

aarifullin commented

2024-04-03 13:56:25 +00:00

If numeric mapping does not exists, then assign id to 0. No explicit assignment led to exception because of Null/Integer conversion when there were no chains yet.
Add check to unit-test.
Regenerate rpcclient for Policy.

* If numeric mapping does not exists, then assign id to 0. No explicit assignment led to exception because of `Null/Integer` conversion when there were no chains yet. * Add check to unit-test. * Regenerate rpcclient for Policy.

aarifullin added the

bug

label 2024-04-03 13:56:25 +00:00

aarifullin requested review from storage-core-committers 2024-04-03 13:58:12 +00:00

aarifullin requested review from storage-core-developers 2024-04-03 13:58:20 +00:00

aarifullin changed title from ~~policy: Fix mapToNumeric helper~~ to WIP: policy: Fix mapToNumeric helper

2024-04-03 14:19:17 +00:00

aarifullin force-pushed fix/policy_map_key from d412991452 to 465d0b10f1

2024-04-03 14:45:04 +00:00

Compare

aarifullin changed title from ~~WIP: policy: Fix mapToNumeric helper~~ to policy: Fix mapToNumeric helper

2024-04-03 14:46:23 +00:00

aarifullin changed title from ~~policy: Fix mapToNumeric helper~~ to policy: Fix IteratorChainsByPrefix

2024-04-03 14:49:12 +00:00

acid-ant reviewed 2024-04-03 18:40:05 +00:00

policy/policy_contract.go Outdated

					
				@ -246,1 +246,3 @@

					id, _ := mapToNumeric(ctx, entity, []byte(entityName))

					id, mappingExists := mapToNumeric(ctx, entity, []byte(entityName))

					if !mappingExists {

						// All exising numeric mappings starts with 1. So, if id is set to 0, then

acid-ant commented

2024-04-03 18:40:05 +00:00

exising -> existing

acid-ant commented

2024-04-03 18:40:42 +00:00

Please fix commit message.

👍 1

aarifullin force-pushed fix/policy_map_key from 465d0b10f1 to 4ab2e59333

2024-04-04 07:26:43 +00:00

Compare

acid-ant approved these changes 2024-04-04 07:40:06 +00:00

fyrchik reviewed 2024-04-04 08:00:46 +00:00

policy/policy_contract.go Outdated

					
				@ -244,3 +244,3 @@

				func IteratorChainsByPrefix(entity Kind, entityName string, prefix []byte) iterator.Iterator {

					ctx := storage.GetReadOnlyContext()

					id, _ := mapToNumeric(ctx, entity, []byte(entityName))

					id, mappingExists := mapToNumeric(ctx, entity, []byte(entityName))

fyrchik commented

2024-04-03 15:23:09 +00:00

Why can we return 0 in the first place?

Why can we return `0` in the first place?

fyrchik commented

2024-04-04 08:00:41 +00:00

Why can we return 0 from mappingExists in the first place?

Why can we return 0 from `mappingExists` in the first place?

aarifullin commented

2024-04-04 08:49:45 +00:00

mappingExists returns false, if entityType + entityName was never mapped.

When we deploy contract

	storage.Put(ctx, counterKey, 0)

When we create a mapping

func mapToNumericCreateIfNotExists(ctx storage.Context, kind Kind, name []byte) int {
	mKey := mapKey(kind, name)
	numericID := storage.Get(ctx, mKey)
	if numericID == nil {
		counter := storage.Get(ctx, counterKey).(int)
		counter++
		storage.Put(ctx, counterKey, counter)
		storage.Put(ctx, mKey, counter)
		return counter
	}
	return numericID.(int)
}

So, as you can see, all mappings start from 1 <- counter++. Here I use 0 to intentionally fail iteration: we suppose mapping to 0 does not exist -> no chains for 0 -> iteration over empty list

`mappingExists` returns false, if `entityType + entityName` was never mapped. When we deploy contract ```go storage.Put(ctx, counterKey, 0) ``` When we create a mapping ```go func mapToNumericCreateIfNotExists(ctx storage.Context, kind Kind, name []byte) int { mKey := mapKey(kind, name) numericID := storage.Get(ctx, mKey) if numericID == nil { counter := storage.Get(ctx, counterKey).(int) counter++ storage.Put(ctx, counterKey, counter) storage.Put(ctx, mKey, counter) return counter } return numericID.(int) } ``` So, as you can see, all mappings start from `1` <- `counter++`. Here I use `0` to intentionally *fail* iteration: we suppose mapping to `0` does not exist -> no chains for `0` -> iteration over empty list

fyrchik commented

2024-04-04 11:18:20 +00:00

Again, 4ab2e59333/policy/policy_contract.go (L118) here we could return 0, false explicitly

Null/Integer conversion is most likely a bug in the compiler, explicit returns should fix this.

Again, https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/4ab2e593336cd76c17e3d8dae12e9320ccdb8891/policy/policy_contract.go#L118 here we could return `0, false` explicitly `Null/Integer` conversion is most likely a bug in the compiler, explicit returns should fix this.

aarifullin commented

2024-04-04 12:42:21 +00:00

Okay. Before, I did it like you said but then I concluded it is better to explain with a comment that iterating over 0 is fine.

Null/Integer conversion is most likely a bug in the compiler, explicit returns should fix this.

Yes. That's correct. So, it really makes sense to return changes to prevent occurring of this bug

Okay. Before, I did it like you [said](https://git.frostfs.info/TrueCloudLab/frostfs-contract/commit/d41299145270d01d1e90eb9d1dc93f36ca145cd3) but then I concluded it is better to explain with a comment that iterating over `0` is fine. > Null/Integer conversion is most likely a bug in the compiler, explicit returns should fix this. Yes. That's correct. So, it really makes sense to return changes to prevent occurring of this bug

aarifullin commented

2024-04-04 12:44:34 +00:00

I returned back the change (but made it return 0, false instead explicit assignment at the very beginning). Please, check this out

I returned back the change (but made it `return 0, false` instead explicit assignment at the very beginning). Please, check this out

fyrchik commented

2024-04-04 15:02:38 +00:00

Will be fixed, but not soon https://github.com/nspcc-dev/neo-go/pull/3401

🚀 1