Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/vub for meta on chain and method overloads #451

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

carpawell
Copy link
Member

No description provided.

@carpawell carpawell self-assigned this Dec 11, 2024
@carpawell carpawell marked this pull request as draft December 11, 2024 16:35
@@ -277,7 +277,7 @@ func SubmitObjectPut(metaInformation []byte, sigs [][]interop.Signature) {
}
}
vub := getFromMap(metaMap, "validuntil").(int)
if vub <= ledger.CurrentIndex() {
if vub <= contract.Call(storage.Get(ctx, netmapContractKey).(interop.Hash160), "epoch", contract.ReadOnly).(int) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still expect blocks here. Epochs are too coarse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you have problems calculating it on different nodes, but this can be solved in different ways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And epochs are not much different here.

Copy link
Member Author

@carpawell carpawell Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still expect blocks here. Epochs are too coarse.

Well, maybe. Still, we need to sync them somehow.

I know you have problems calculating it on different nodes, but this can be solved in different ways.

Like the last block in the current epoch + const? I don't mind but I have nothing to inspire me here. Epochs are just a little bit more closer to our storage.

And epochs are not much different here.

In the node it is currently creation epoch + const.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if it's in blocks then that's the value to use, isn't it? One node sets it, others are either OK or not with it (usually they're OK).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to drop this patch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to drop this patch.

i don't mind but what block to use in the nodes then? i can only think of

func LastEpochBlock() int {
+ const and hope it is not gonna tick while signatures are exchanged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use current + something, it is exchanged between nodes, so it's not a problem for other nodes to use the value.

@carpawell carpawell force-pushed the fix/vub-for-meta-on-chain branch from 54ecc4c to 841929d Compare December 11, 2024 16:41
@carpawell
Copy link
Member Author

carpawell commented Dec 11, 2024

@roman-khimov, I do not like the second commit, but this is how we can fix it. Now, I know that overloads exist, and I want them all to be overloaded, not like they are partly now. But to be fully honest, put and putNamed are the same method;"", "" makes them the same. However, with this PR, it is more correct, but I do not like them still.

@carpawell carpawell marked this pull request as ready for review December 11, 2024 16:46
@roman-khimov
Copy link
Member

Regarding overloads -- it's ok for me for now.

@roman-khimov
Copy link
Member

In general, overloaded put would be better, but we already use putNamed a lot, it can't be changed easily.

@roman-khimov
Copy link
Member

Although you can have proper set of puts and putNamed at the same time.

@carpawell carpawell force-pushed the fix/vub-for-meta-on-chain branch 2 times, most recently from baa8491 to dd6fb9a Compare December 12, 2024 12:52
@carpawell
Copy link
Member Author

carpawell commented Dec 12, 2024

After a small talk with @roman-khimov, we are making overloads now and deprecating PutNamed.

It is now:
put(container, signature, publicKey, token)
put(container, signature, publicKey, token, name, zone)
put(container, signature, publicKey, token, name, zone, meta-on-chain)

I suggest all the clients use the last one, it does not duplicate any logic: if you read name and zone, it is non-empty strings. If you asked to enable meta-not-chain, it is read and not-false. Defaults behave as the first one.

I do not want to have
put(container, signature, publicKey, token, meta-on-chain), it would be too many methods IMO.

@carpawell
Copy link
Member Author

First commit dropped BTW.

The old (already released) ways are kept. Not it is possible to call "put" as
Put (4 args), PutNamed (6 args) and PutMeta (7 args). On the client side, it is
acceptable to always call "put" with 7 args and just be sure that the last 3 are
meaningful.

Signed-off-by: Pavel Karpy <[email protected]>
@carpawell carpawell force-pushed the fix/vub-for-meta-on-chain branch from dd6fb9a to de118c4 Compare December 12, 2024 13:34
@roman-khimov roman-khimov merged commit 362366f into master Dec 12, 2024
8 checks passed
@roman-khimov roman-khimov deleted the fix/vub-for-meta-on-chain branch December 12, 2024 13:55
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 18, 2024
It now may have additional arg that enables meta-on-chain support and be named,
see nspcc-dev/neofs-contract#451. Closes #2877.

Signed-off-by: Pavel Karpy <[email protected]>
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 18, 2024
It is more natural for on-chain operations. Also, it requires handling sync
problems when different blocks are used for the same objects on different nodes:
two signatures are always attached with epoch duration difference.
Refs nspcc-dev/neofs-contract#451.

Signed-off-by: Pavel Karpy <[email protected]>
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 18, 2024
It is more natural for on-chain operations. Also, it requires handling sync
problems when different blocks are used for the same objects on different nodes:
two signatures are always attached with epoch duration difference.
Refs nspcc-dev/neofs-contract#451.

Signed-off-by: Pavel Karpy <[email protected]>
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 23, 2024
It now may have additional arg that enables meta-on-chain support and be named,
see nspcc-dev/neofs-contract#451. Closes #2877.

Signed-off-by: Pavel Karpy <[email protected]>
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 23, 2024
It is more natural for on-chain operations. Also, it requires handling sync
problems when different blocks are used for the same objects on different nodes:
two signatures are always attached with epoch duration difference.
Refs nspcc-dev/neofs-contract#451.

Signed-off-by: Pavel Karpy <[email protected]>
carpawell added a commit to nspcc-dev/neofs-node that referenced this pull request Dec 23, 2024
It is more natural for on-chain operations. Also, it requires handling sync
problems when different blocks are used for the same objects on different nodes:
two signatures are always attached with epoch duration difference.
Refs nspcc-dev/neofs-contract#451.

Signed-off-by: Pavel Karpy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants