Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Implement native Notary contract #3178

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Implement native Notary contract #3178

wants to merge 58 commits into from

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Mar 19, 2024 •
edited
Loading

Description

Implement native Notary contract.

Close #2897.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Native Gas contract unit tests;
  • Native Notary contract unit tests.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules:

AnnaShaleva and others added 8 commits March 6, 2024 12:00
@AnnaShaleva
Implement NotaryAssisted transaction attribute
6da4ae2 
Close #2896. Use a stub for native Notary contract hash since this
contract is not implemented yet. Thus, technically, NotaryAssisted
attribute verification will always fail on real network until native
Notary is implemented.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Payloads: add doc to CalculateNetworkFee method of NotaryAssisted att…
acec1b0 
…ribute

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Native: add NotaryAssisted attributes handler to Gas OnPersist
1508d4f 
Transactions network fee should be split between Primary node and Notary
nodes.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Payloads: adjust comment to NotaryAssisted attribute
8b547c9 
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Payloads: temporary use hard-coded Notary contract hash
b16a28c 
Once Notary contract is implemented, this hash will be replaced by a
proper Notary contract hash. The exact value won't be changed since
Notary contract has constant hash as any other native contract.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@Jim8y
Merge branch 'master' into notary-assisted
00b54ff
@AnnaShaleva
Payloads: replace hard-coded Notary hash value with calculated one
24135ff 
No functional changes, just a refactoring for better code readability.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Native: implement native Notary contract
77ffe09 
Close #2897. Depends on #3175.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@vncoelho
Copy link
Member

Is this PR to be merged now or after next release?
I was planning to review current release and then merge these next feature.

@roman-khimov
Copy link
Contributor

Is this PR to be merged now or after next release?

https://github.com/neo-project/neo/milestone/2

cschuchardt88
cschuchardt88 requested changes Mar 21, 2024
View reviewed changes
@dusmart
Copy link

dusmart commented Mar 21, 2024

This contract has a Verify method which is very dangerous.

neo/src/Neo/SmartContract/Native/Notary.cs

Line 95 in 77ffe09

private bool Verify(ApplicationEngine engine, byte[] sig)

In this method, we verify that tx.Signers[1] has enough balance stored in the contract. Plus, we verify that a signature from any notary is provided.

Here is an attack vector. A malicious notary can sign as many transactions as it wants and then publish at most 500 txs on chain while tx.Signers[1] has a little balance only sufficient for one tx.

If it's not a malicious notary, someone could still get a chance to make the notary sign something multiple times and cache them then publish them at once.

By the way, do you know how many core modules will be affected by this notary feature? Is this the last PR?

vncoelho
vncoelho reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am not seeing the part of the code we discussed on Discord, @AnnaShaleva, you mentioned about a delegated powered notary signing.
"The contract itself don't send the transactions. It's a designated Notary node who is powered to send transactions on behalf of notary service users. "

I want to check that because it should be, at least, similar to the oracles design.

@AnnaShaleva AnnaShaleva mentioned this pull request Mar 21, 2024
Merged
12 tasks
@AnnaShaleva
Native: fix typo in the exception message
cb4bdda 
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Native: use more syntactic sugar
8f9f5f7 
No functional changes, just a refactoring.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Copy link
Member Author

By the way, do you know how many core modules will be affected by this notary feature? Is this the last PR?

To be able to process existing NeoFS chains that use Notary - yes, it's the last PR. But ideally in future we'd like to implement P2PNotaryRequest payload with pool (ref. nspcc-dev/neo-go#1546) and Notary service plugin (ref. nspcc-dev/neo-go#1547). We'll add related issues to neo-project later.

"The contract itself don't send the transactions. It's a designated Notary node who is powered to send transactions on behalf of notary service users. "
I want to check that because it should be, at least, similar to the oracles design.

It's a part of Notary service plugin. See the example implementation in https://github.com/nspcc-dev/neo-go/blob/master/pkg/services/notary/notary.go.

vang1ong7ang
vang1ong7ang suggested changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@vang1ong7ang vang1ong7ang left a comment

Choose a reason for hiding this comment

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

as @dusmart said , verify method should be used with extreme caution. and is there a fork introduced?

@roman-khimov
Copy link
Contributor

verify method should be used with extreme caution

That's a valid concern, but there are multiple ways to handle it, we'll get to it after #3175 merge.

shargon
shargon reviewed Mar 24, 2025
View reviewed changes
src/Neo/SmartContract/Native/Notary.cs Outdated
{
if (!engine.CheckWitnessInternal(from)) throw new InvalidOperationException(string.Format("Failed to check witness for {0}", from.ToString()));
var receive = to is null ? from : to;
var deposit = GetDepositFor(engine.SnapshotCache, from) ?? throw new InvalidOperationException(string.Format("Deposit of {0} is null", from.ToString()));
Copy link
Member

Choose a reason for hiding this comment

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

Remove can return the deposit (interoperableClone) not related to the storage, and avoid get/remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Remove can return the deposit

But we're not interested in return value, it's just not needed, we're OK with the fact that deposit is removed.

and avoid get/remove

We need to perform some validation checks before removal, otherwise it's illegal. Also, it's nice to have separate get/remove functions because they are reused from several Notary methods.

@AnnaShaleva AnnaShaleva requested a review from shargon March 24, 2025 18:29
AnnaShaleva and others added 5 commits March 25, 2025 17:33
@AnnaShaleva
UnitTests: remove NotaryAssisted fees unit test
fb1a9be 
This test requires Notary nodes to be properly designated via native
RoleManagement contract, it's hard to mock this behaviour during test,
I wasn't able to properly do it.

This test may be restored once we have better testing system.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Notary: replace exceptions with false return for withdraw
5243124 
Ref.
#3178 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Notary: refactor variable definition
4f11a48 
No functional changes, ref.
#3178 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva
Merge branch 'master' into notary-contract
9705f5a
@Jim8y
Merge branch 'master' into notary-contract
bda7ff7
@AnnaShaleva
Copy link
Member Author

@neo-project/core please review one more time, all conversations should be resolved now.

@Jim8y Jim8y requested review from Copilot and removed request for dusmart and vang1ong7ang April 2, 2025 11:29
Copilot
Copilot AI reviewed Apr 2, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the native Notary contract, which adds functionality for multisignature transaction assistance and deposit management. Key changes include:

  • New native Notary contract implementation with methods for verifying transactions, handling GAS deposits, and rewarding notary nodes.
  • New unit tests for the Notary contract and updated test utilities and extension methods.
  • Updates to native contract references and test fixtures to include the Notary contract.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Neo.UnitTests/TestUtils.Transaction.cs Introduces a new overload to create transactions with an extra signer.
tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs Adds Notary contract configuration in test setups.
tests/Neo.UnitTests/SmartContract/Native/UT_GasToken.cs Removes an existing test method for Notary-assisted persist, which may affect test coverage.
tests/Neo.UnitTests/Extensions/Nep17NativeContractExtensions.cs Updates extension methods to pass deposit-related data to the transfer call.
tests/Neo.UnitTests/Extensions/NativeContractExtensions.cs Adds an overload for Call to support ApplicationEngine.
src/Neo/SmartContract/Native/Notary.cs Implements the Notary contract with methods for deposit handling, reward calculation, and verification.
src/Neo/SmartContract/Native/NativeContract.cs Registers the new Notary contract in the native contract collection.
Comments suppressed due to low confidence (1)

tests/Neo.UnitTests/SmartContract/Native/UT_GasToken.cs:154

  • The removal of the 'Check_OnPersist_NotaryAssisted' test in UT_GasToken.cs reduces the test coverage for the GAS token's handling of Notary-assisted transactions; please ensure that the new Notary functionality is adequately covered by corresponding tests.
        [TestMethod] public void Check_OnPersist_NotaryAssisted() { ... }

Wi1l-B0t
Wi1l-B0t previously approved these changes Apr 2, 2025
View reviewed changes
@AnnaShaleva
Notary: migrate to Policy.GetAttributeFeeV1 usage
e272a16 
No functional changes, just adopt the #3859.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@AnnaShaleva AnnaShaleva mentioned this pull request Apr 3, 2025
Closed
shargon
shargon reviewed Apr 3, 2025
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

My review #3870 some styles and one concern with null balance

This was referenced Apr 3, 2025
Merged
Open
@shargon
Review Notary (#3870)
46622db 
* Update Notary.cs

* Update Notary.cs

* Update Notary.cs

* fix

* clean

* Clean

* Update src/Neo/SmartContract/Native/Notary.cs
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@shargon shargon

@dusmart dusmart

Copilot code review Copilot

@vang1ong7ang vang1ong7ang

@Wi1l-B0t Wi1l-B0t

@Jim8y Jim8y

@roman-khimov roman-khimov

@superboyiii superboyiii

@vncoelho vncoelho

@Hecate2 Hecate2

@cschuchardt88 cschuchardt88

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Hardfork Waiting for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Notary native contract
10 participants
@AnnaShaleva @vncoelho @roman-khimov @dusmart @shargon @vang1ong7ang @Wi1l-B0t @cschuchardt88 @Jim8y @Hecate2