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

Set attribute fee #2916

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Set attribute fee #2916

merged 7 commits into from
Oct 10, 2023

Conversation

shargon
Copy link
Member

@shargon shargon commented Sep 19, 2023

First part of #2913
Idea of @AnnaShaleva

@roman-khimov
Copy link
Contributor

roman-khimov commented Sep 19, 2023

A nice generalization. I think NotaryAssisted (from #2896) will fit into this scheme easily as well (the value could be removed from the Notary contract, doesn't matter much which contract stores it). @AnnaShaleva?

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Really nice idea, I like it!

src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved
src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved
src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved
src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved
src/Neo/Network/P2P/Payloads/Conflicts.cs Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

I think NotaryAssisted (from #2896) will fit into this scheme easily as well

Sure.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

LGTM.

src/Neo/Network/P2P/Payloads/Conflicts.cs Show resolved Hide resolved
@shargon
Copy link
Member Author

shargon commented Sep 20, 2023

@superboyiii could you test it?

@roman-khimov roman-khimov mentioned this pull request Sep 20, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 21, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 21, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 21, 2023
@superboyiii
Copy link
Member

@superboyiii could you test it?

Sure

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023
@superboyiii
Copy link
Member

superboyiii commented Oct 10, 2023

Tested. Good enough, let's move on. @shargon

@shargon shargon merged commit f5e257c into master Oct 10, 2023
1 of 2 checks passed
@shargon shargon deleted the set-attr-fee branch October 10, 2023 09:19
@vncoelho
Copy link
Member

@shargon, it looks like this is the same case as KECAK.
@superboyiii, this PR need fork tag.

If this is merged like this without tag there are possibilities for attacks. Who will be responsible to monitor all CN upgrade for that and check if the syscal was not used?
This is dangerous.

@vang1ong7ang @dusmart

@vncoelho
Copy link
Member

#2925

@shargon
Copy link
Member Author

shargon commented Oct 10, 2023

This is dangerous.

This is a fix for a public exploit.

@vncoelho
Copy link
Member

The origin of the exploit is also another problem as you known. The Conflict PR was not ready.

At least, upgrade the nodes now and release a version that CN will upgrade. Do not postpone it.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 10, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 10, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 10, 2023
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this pull request Oct 10, 2023
ConflictsFee logic was replaced by the generic attribute fee mechanism
implemented in neo-project#2916.

Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 12, 2023
shargon added a commit that referenced this pull request Nov 8, 2023
* Ledger: change conflict records storage scheme

This commit is a part of #2907: it implements conflict records storage scheme discribed
in #2907 (comment).

The short scheme description:

Do not store the list of conflicting signers in the Ledger's conflict record value.
Instead, put each conflicting signer in the conflict record key so that the
reculting key is: {Prefix_Transaction, <conflict hash>, <signer>}, and the value
is {<block index>}.

As an optimisation, for each conflict record store the dummy stub where the
key is {Prefix_Transaction, <conflict hash>} and the value is {<block index>}. This optimisation
allows to reduce DB read requests during newly-pooled transaction verification for
those transactions that do not have any on-chained conflicts.

Also, IsTraceableBlock check is added for on-chain conflicts verification. It is
needed to avoid situations when untraceable on-chained conflict affects the
newly-pooled transaction verification.

Signed-off-by: Anna Shaleva <[email protected]>

* UT_MemoryPool: remove unused test

Malicious_OnChain_Conflict was constructed incorrectly (see the comment in the end of
the test) and was replaced by the proper TestMaliciousOnChainConflict test in
UT_Blockchain.cs way back ago in
0c06c91.

Signed-off-by: Anna Shaleva <[email protected]>

* Policy: introduce fee for Conflicts attribute

This commit implements Conflicts attribute policy described in
#2907 (comment).

Signed-off-by: Anna Shaleva <[email protected]>

* *: remove remnants of ConflictsFee in native Policy

ConflictsFee logic was replaced by the generic attribute fee mechanism
implemented in #2916.

Signed-off-by: Anna Shaleva <[email protected]>

* Native: do not remove malicious conflict records during OnPersist

It's OK to keep them and save O(n) operations during OnPersist. The
storage taken by these malicious conflict records is properly paid
anyway. See the discussion under #2913 (comment).

Signed-off-by: Anna Shaleva <[email protected]>

* Properly rewrite previously added malicious conflict if it's in the storage

`engine.Snapshot.Add` doesn't allow to rewrite storage entity if it's already exist.
Thus, we firstly need to remove it and afterwards to add the updated entity.

Signed-off-by: Anna Shaleva <[email protected]>

* Throw proper exception if TestMaliciousOnChainConflict fails

Signed-off-by: Anna Shaleva <[email protected]>

* Optimize conflicts records storing

Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add.
Ref. #2913 (comment).

Signed-off-by: Anna Shaleva <[email protected]>

---------

Signed-off-by: Anna Shaleva <[email protected]>
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Jimmy <[email protected]>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 20, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 20, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 20, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 20, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 21, 2023
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 21, 2023
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.

5 participants