Implement NotaryAssisted transaction attribute #3175
Conversation
Close neo-project#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 <[email protected]>
Port neo-project/neo#3175. Ref. neo-project/neo#2896. Signed-off-by: Anna Shaleva <[email protected]>
Port neo-project/neo#3175. Ref. neo-project/neo#2896. Signed-off-by: Anna Shaleva <[email protected]>
|
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 |
…ribute Signed-off-by: Anna Shaleva <[email protected]>
Sure, fixed. |
Transactions network fee should be split between Primary node and Notary nodes. Signed-off-by: Anna Shaleva <[email protected]>
Signed-off-by: Anna Shaleva <[email protected]>
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 <[email protected]>
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. |
AnnaShaleva
requested review from
shargon,
cschuchardt88,
superboyiii and
NGDAdmin
Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
notary-assisted
branch
from
eded6bd to
c229cd5
Compare
D hardfork was occupied by 3.7.5, thus use the next available one. Signed-off-by: Anna Shaleva <[email protected]>
|
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 |
No functional changes, just build fixes required by updated master branch. Signed-off-by: Anna Shaleva <[email protected]>
|
@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. |
Signed-off-by: Anna Shaleva <[email protected]>
src/Neo/Hardfork.cs
Outdated
| @@ -16,6 +16,7 @@ public enum Hardfork : byte | |||
| HF_Aspidochelone, | |||
| HF_Basilisk, | |||
| HF_Cockatrice, | |||
| HF_Domovoi | |||
| HF_Domovoi, | |||
| HF_Echidna, | |||
There was a problem hiding this comment.
I'll refactor this code after the merge of #3454.
Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
notary-assisted
branch
from
ae6ad54 to
ec4e3d6
Compare
Related to additional storage entries added by the previous changes. Signed-off-by: Anna Shaleva <[email protected]>
| var notaryAssisted = tx.GetAttribute<NotaryAssisted>(); | ||
| if (notaryAssisted is not null) | ||
| { | ||
| totalNetworkFee -= (notaryAssisted.NKeys + 1) * Policy.GetAttributeFee(engine.SnapshotCache, (byte)notaryAssisted.Type); |
There was a problem hiding this comment.
@AnnaShaleva AttributeFee in memory pool can be 1, and during the block persistence can be changed to 2, this could underflow totalNetworkFee and produce a denial of service.
| /// <param name="attributeType">Attribute type excluding <see cref="TransactionAttributeType.NotaryAssisted"/></param> | ||
| /// <param name="value">Attribute fee value</param> | ||
| /// <returns>The fee for attribute.</returns> | ||
| [ContractMethod(Hardfork.HF_Echidna, CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States, Name = "setAttributeFee")] |
There was a problem hiding this comment.
I think we don't need to change the manifest if only the logic changed, we can do it in the same method
| private uint GetAttributeFee(DataCache snapshot, byte attributeType, bool allowNotaryAssisted) | ||
| { | ||
| if (!Enum.IsDefined(typeof(TransactionAttributeType), attributeType) || (!allowNotaryAssisted && attributeType == (byte)(TransactionAttributeType.NotaryAssisted))) | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
| throw new InvalidOperationException(); | |
| throw new InvalidOperationException($"Invalid value {attributeType} of {nameof(attributeType)}"); |
| { | ||
| if (!Enum.IsDefined(typeof(TransactionAttributeType), attributeType)) throw new InvalidOperationException(); | ||
| if (!Enum.IsDefined(typeof(TransactionAttributeType), attributeType) || (!allowNotaryAssisted && attributeType == (byte)(TransactionAttributeType.NotaryAssisted))) | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
| throw new InvalidOperationException(); | |
| throw new InvalidOperationException($"Invalid value {attributeType} of {nameof(attributeType)}"); |
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
How Has This Been Tested?
Checklist: