policy: Shorten policy contract keys #71

Merged
fyrchik merged 1 commit from aarifullin/frostfs-contract:feature/61-shorten_policy_keys into master 2024-01-30 12:27:58 +00:00
2 changed files with 69 additions and 9 deletions
Showing only changes of commit a8c75874a7 - Show all commits

View file

@ -6,6 +6,8 @@
|------------------------------------------|--------|-----------------------------------|
| 'c' + uint16(len(container)) + container | []byte | Namespace chain |
| 'n' + uint16(len(namespace)) + namespace | []byte | Container chain |
| 'm' + entity name (namespace/container) | []byte | Mapped name to an encoded number |
| 'counter' | uint64 | Integer counter used for mapping |
*/

View file

@ -22,6 +22,11 @@ const (
ownerKeyPrefix = 'o'
)
const (
mappingKeyPrefix = 'm'
counterKey = "counter"
)
const (
// ErrNotAuthorized is returned when the none of the transaction signers
// belongs to the list of autorized keys.
@ -44,6 +49,7 @@ func _deploy(data any, isUpdate bool) {
}
storage.Put(ctx, []byte{ownerKeyPrefix}, args.Admin)
}
storage.Put(ctx, counterKey, 0)
}
dkirillov marked this conversation as resolved Outdated

Isn't int in contracts is uint64 anyway and we can skip conversion?

Isn't `int` in contracts is `uint64` anyway and we can skip conversion?

I think you're right. I have replaced uint64 by int

I think you're right. I have replaced `uint64` by `int`
func checkAuthorization(ctx storage.Context) {
@ -79,24 +85,61 @@ func getAdmin(ctx storage.Context) interop.Hash160 {
return storage.Get(ctx, []byte{ownerKeyPrefix}).(interop.Hash160)
}
func storageKey(prefix Kind, entityName string, name []byte) []byte {
ln := len(entityName)
key := append([]byte{byte(prefix)}, byte(ln&0xFF), byte(ln>>8))
key = append(key, entityName...)
func storageKey(prefix Kind, counter int, name []byte) []byte {

Why not calculate this inside storageKey()
The invariant is hard to enforce.
Otherwise, let's rename entityName parameter, it is counter now?

Why not calculate this inside `storageKey()` The invariant is hard to enforce. Otherwise, let's rename `entityName` parameter, it is `counter` now?

mapToNumeric could return int, this would solve all mentioned problems.

`mapToNumeric` could return `int`, this would solve all mentioned problems.

You've asked

Why not calculate this inside storageKey()

Because:

This method is also invoked by GetChain that yields read-only context. If we consider your suggestion, then it leads to a situation when we need to make storageKey to determine whether it can create mapping or not, i.e. write to the storage or not (storage.Put) -> we need to pass two more params: ctx and isReadOnlyCtx: actually, I tried to avoid this

Fusing these methods would look like that:

func storageKeyAlt(ctx storage.Context, isReadOnlyCtx bool, prefix Kind, entityName string, name []byte) []byte {
	mKey := append([]byte{mappingKeyPrefix}, entityName...)
	var mapped int
	numericID := storage.Get(ctx, mKey)
	if numericID == nil {
		if isReadOnlyCtx {
			return nil
		}
		counter := storage.Get(ctx, counterKey).(int)
		counter++
		storage.Put(ctx, counterKey, counter)
		storage.Put(ctx, mKey, counter)
		mapped = counter
	} else {
		mapped = numericID.(int)
	}

	key := append([]byte{byte(prefix)}, common.ToFixedWidth64(mapped)...)
	return append(key, name...)
}

Are we okay with that?

You've asked > Why not calculate this inside storageKey() Because: This method is also invoked by `GetChain` that yields read-only context. If we consider your suggestion, then it leads to a situation when we need to make `storageKey` to determine whether it can create mapping or not, i.e. write to the storage or not (`storage.Put`) -> we need to pass two more params: `ctx` and `isReadOnlyCtx`: actually, I tried to avoid this Fusing these methods would look like that: ```go func storageKeyAlt(ctx storage.Context, isReadOnlyCtx bool, prefix Kind, entityName string, name []byte) []byte { mKey := append([]byte{mappingKeyPrefix}, entityName...) var mapped int numericID := storage.Get(ctx, mKey) if numericID == nil { if isReadOnlyCtx { return nil } counter := storage.Get(ctx, counterKey).(int) counter++ storage.Put(ctx, counterKey, counter) storage.Put(ctx, mKey, counter) mapped = counter } else { mapped = numericID.(int) } key := append([]byte{byte(prefix)}, common.ToFixedWidth64(mapped)...) return append(key, name...) } ``` Are we okay with that?

Doesn't look nice either, but we could just move common.ToFixedWidth64invocation: so that storageKey accepts int returned from mapToNumeric.

Doesn't look nice either, but we could just move `common.ToFixedWidth64`invocation: so that `storageKey` accepts `int` returned from `mapToNumeric`.

Alright. Since mapping methods return int and storageKey recieves int

Alright. Since mapping methods return `int` and `storageKey` recieves `int`
key := append([]byte{byte(prefix)}, common.ToFixedWidth64(counter)...)
return append(key, name...)
}
dkirillov marked this conversation as resolved
Review

Shouldn't we simplify this to key := append([]byte{byte(prefix)}, entityName...)

Shouldn't we simplify this to `key := append([]byte{byte(prefix)}, entityName...)`
Review

Appending entityName's length wasn't idea of mine (it has been introduced by @fyrchik), but I think it is fine because it helps to determine where a chain ID starts in a storage key.

Appending `entityName`'s length wasn't idea of mine (it has been introduced by @fyrchik), but I think it is fine because it helps to determine where a chain ID starts in a storage key.
Review

entityName now always 8 bytes long. So we can save two bytes here if I understand it correctly

`entityName` now always 8 bytes long. So we can save two bytes here if I understand it correctly
Review

Alright. I've removed this append

Alright. I've removed this append
// mapToNumeric maps a name to a number. That allows to keep more space in
// a storage key shortening long names. Short entity
// names are also mapped to prevent collisions in the map.
func mapToNumeric(ctx storage.Context, name []byte) (mapped int, mappingExists bool) {
mKey := append([]byte{mappingKeyPrefix}, name...)
numericID := storage.Get(ctx, mKey)
if numericID == nil {
return
}
mapped = numericID.(int)
mappingExists = true
return
dkirillov marked this conversation as resolved Outdated

I'm not sure that we can use plain namespace if its length is less than 8 bytes because it seems we can conflict with mapped one.
Let's consider namespace with name a, it's []byte representation is []byte{97}, but we will get the same representation for 97 container that length is greater 8 as I can undersand

I'm not sure that we can use plain `namespace` if its length is less than 8 bytes because it seems we can conflict with mapped one. Let's consider namespace with name `a`, it's []byte representation is `[]byte{97}`, but we will get the same representation for 97 container that length is greater 8 as I can undersand

You're absolutely right. I haven't considered this collision. Alright, I'll remove this out then

You're absolutely right. I haven't considered this collision. Alright, I'll remove this out then

Fixed. Since all names are mapped to numbers

Fixed. Since **all** names are mapped to numbers
}
// mapToNumericCreateIfNotExists maps a name to a number. That allows to keep
dkirillov marked this conversation as resolved Outdated

Can be just return common.ToFixedWidth64(mapped)

Can be just `return common.ToFixedWidth64(mapped)`

Fixed

Fixed
// more space in a storage key shortening long names. Short entity
// names are also mapped to prevent collisions in the map.
// If a mapping cannot be found, then the method creates and returns it.
// mapToNumericCreateIfNotExists is NOT applicable for a read-only context.
func mapToNumericCreateIfNotExists(ctx storage.Context, name []byte) int {
mKey := append([]byte{mappingKeyPrefix}, 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)
}
func AddChain(entity Kind, entityName string, name []byte, chain []byte) {
ctx := storage.GetContext()
checkAuthorization(ctx)
key := storageKey(entity, entityName, name)
entityNameBytes := mapToNumericCreateIfNotExists(ctx, []byte(entityName))
key := storageKey(entity, entityNameBytes, name)
storage.Put(ctx, key, chain)
}
func GetChain(entity Kind, entityName string, name []byte) []byte {
ctx := storage.GetReadOnlyContext()
key := storageKey(entity, entityName, name)
entityNameBytes, exists := mapToNumeric(ctx, []byte(entityName))
if !exists {
panic("not found")
}
key := storageKey(entity, entityNameBytes, name)
data := storage.Get(ctx, key).([]byte)
if data == nil {
panic("not found")
@ -109,7 +152,12 @@ func RemoveChain(entity Kind, entityName string, name []byte) {
ctx := storage.GetContext()
checkAuthorization(ctx)
key := storageKey(entity, entityName, name)
entityNameBytes, exists := mapToNumeric(ctx, []byte(entityName))
if !exists {
return
}
key := storageKey(entity, entityNameBytes, name)
storage.Delete(ctx, key)
}
@ -117,7 +165,12 @@ func RemoveChainsByPrefix(entity Kind, entityName string, name []byte) {
ctx := storage.GetContext()
checkAuthorization(ctx)
key := storageKey(entity, entityName, name)
entityNameBytes, exists := mapToNumeric(ctx, []byte(entityName))
if !exists {
return
}
key := storageKey(entity, entityNameBytes, name)
it := storage.Find(ctx, key, storage.KeysOnly)
for iterator.Next(it) {
storage.Delete(ctx, iterator.Value(it).([]byte))
@ -142,7 +195,12 @@ func ListChainsByPrefix(entity Kind, entityName string, prefix []byte) [][]byte
result := [][]byte{}
keyPrefix := storageKey(entity, entityName, prefix)
entityNameBytes, exists := mapToNumeric(ctx, []byte(entityName))
if !exists {
return result
}
keyPrefix := storageKey(entity, entityNameBytes, prefix)
it := storage.Find(ctx, keyPrefix, storage.ValuesOnly)
for iterator.Next(it) {
result = append(result, iterator.Value(it).([]byte))