neo-project
Implement NotaryAssisted transaction attribute
Description
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.
Type of change
- [X] New feature (non-breaking change which adds functionality)
How Has This Been Tested?
- [X] A group of unit tests for NotaryAssisted attribute.
Checklist:
- [X] My code follows the style guidelines of this project
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [X] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [X] I have added tests that prove my fix is effective or that my feature works
- [X] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published in downstream modules:
- [ ] https://github.com/neo-project/neo-devpack-dotnet/pull/983
- [ ] https://github.com/neo-project/neo-modules/pull/884
I'm not allowed to request review, set labels or resolve conversations. Maybe someone from neo-project with access rights can allow me at least to request review?
I'm not allowed to request review, set labels or resolve conversations. Maybe someone from neo-project with access rights can allow me at least to request review?
You didn't accept your role xD, I sent you an invitation time ago... Time to review your email? 😄
Hi @AnnaShaleva , we are trying to add as much comments in the core as possible, may you please add comments to explain CalculateNetworkFee.
may you please add comments to explain CalculateNetworkFee.
Sure, fixed.
GAS.OnPersist change is missing for the attribute.
Completely forgot about it, fixed.
Maybe this PR we merge after next release.
This PR doesn't hurt because currently in real network NotaryAssisted attribute is always invalid since transaction must have Notary contract as a signer, and Notary contract is not implemented yet.
Thus, I vote to merge it before 3.7. What do you think?
It goes step by step, @vncoelho. What this delay would buy us? The attribute logic is rather simple on its own.
I didn't expect it to be merged in 3.7
It was a part of #2905 since the beginning, it's just that we were busy with Bane. Maybe we should synchronize more often/better on our plans.
and it is already beginning to produce changes in the core
As intended, it's a core feature. Then there will be a new contract in #2897 (rather small one, I'd say). And this set would allow to process notary-enabled chains. Then we can introduce P2P layer of it (but that can wait till 3.8).
maybe we need more discussion on this pr
- Asymmetry between implementation costs and user needs
- It may lead to the modification of mempool, causing an uncertain risk of downtime.
- need more support on gas calculation
Asymmetry between implementation costs and user needs
Not sure what's the problem here, it's 266 lines with tests, not a big one. The topic of "why" has been beaten to death in https://github.com/neo-project/neo/issues/1573, please have a look.
It may lead to the modification of mempool, causing an uncertain risk of downtime.
This attribute changes nothing wrt mempool.
need more support on gas calculation
It's a reward for notary nodes (see https://github.com/neo-project/neo/pull/3172), simple as that.
Asymmetry between implementation costs and user needs
Not sure what's the problem here, it's 266 lines with tests, not a big one. The topic of "why" has been beaten to death in #1573, please have a look.
@roman-khimov the notary impl is split into multiple prs like #3178 . it's not just 266 lines.
I would consider the conflict attribute introduced in #2818 to be part of the notary plan as well
the fact is:
the conflict attr introduced troubles into the neo network ( #2907 ) BUT till now there is ONLY 2 transaction which used this feature. both of the 2 transactions are sent from me, who is an opponent of this feature.
the notary impl is split into multiple prs
Absolutely intentional because no one could handle #2661.
I would consider the conflict attribute introduced in https://github.com/neo-project/neo/pull/2818 to be part of the notary plan as well
Yes and no, it has generic applications as well.
the conflict attr introduced troubles into the neo network
Implementation error, it happens.
ill now there is ONLY 2 transaction which used this feature.
Take a look at some other network like https://github.com/nspcc-dev/neo-go/blob/master/config/protocol.mainnet.neofs.yml.
For me is good if this was implemented
@shargon, the test is added, it works as expected. Please, check out the https://github.com/neo-project/neo/pull/3178/commits/670546971648b0c16f81b6a94a80b697e70242ab.
@Jim8y @vncoelho @cschuchardt88 and everyone interested, we need to move on with this PR. Please, consider reviewing it.
For me is good if this was implemented
@shargon, the test is added, it works as expected. Please, check out the https://github.com/neo-project/neo/pull/3178/commits/670546971648b0c16f81b6a94a80b697e70242ab.
@Jim8y @vncoelho @cschuchardt88 and everyone interested, we need to move on with this PR. Please, consider reviewing it.
I am aligned with @vang1ong7ang and I will wait until his recommendations are all clarified.
I am aligned with @vang1ong7ang and I will wait until his recommendations are all clarified.
@vncoelho, @vang1ong7ang There are replies (https://github.com/neo-project/neo/pull/3175#issuecomment-2019433661, https://github.com/neo-project/neo/pull/3175#issuecomment-2007050274), so could you please specify the exact questions/issues/possible vector of attack that you have in mind right now? So that we can answer properly.
@neo-project/core, let's try to review one more time, if you have any specific test cases or a possible vector of attack on your mind, then please, comment. This PR is ready for review and may be included into 3.8.0.
@AnnaShaleva, now with neo-modules integrated we also need changes from neo-project/neo-modules#884 here.
need changes from https://github.com/neo-project/neo-modules/pull/884 here.
Sure, will port it ASAP.
Also need to move Policy update under Domovoi hardfork to keep our states unchanged, depends on #3290.
We've checked that NeoFS networks are 100% compatible with both Notary contract and NotaryAssisted attribute enabled starting from D hardfork (see the https://github.com/nspcc-dev/neo-go/issues/3465). It means that Notary contract and NotaryAssisted attribute implementation won't change the existing node state. In the last commit I've moved NotaryAssisted attribute under D hardfork.
Fix all the warning please
Thank you, fixed!
UPD: we're aware of the Technical Committee decision to postpone the Notary subsystem implementation. However, we think that it's good to keep this code up-to-date wrt the current master so that it'll be easier for us to review and merge it once the time comes. Hence, I've updated the code to the latest master and moved the NotaryAssisted attribute under the next available hardfork (HF_Echidna).
@neo-project/core let's resume our work on this PR. I've updated it to fetch fresh master, NotaryAssisted attribute will be enabled starting from Echidna.
@Jim8y @vncoelho @cschuchardt88 need your review here, we've agreed to move on with this PR, and we need it to be a part of 3.8.
You have errors that need fixing 1st.
Thanks for pointing that out, it was due to recent testing framework upgrade happened on master. Unit-test is fixed (see the latest commit), nothing is changed in the core logic, but we need to collect approvals from @shargon @nan01ab and @roman-khimov one more time.