Skip to content

Conversation

@carpawell
Copy link
Member

@carpawell carpawell commented Aug 25, 2021

Closes #519.

Implement notary requests subscription and handling mechanisms that can be also
reused for other node -> contract ---*notification*---> alphabet -> contract
flows in the future.

Fix errors/typos in commentaries. Delete spaces in logs' keys.

@carpawell carpawell added enhancement Improving existing functionality neofs-ir Inner Ring node application issues neofs-storage Storage node application issues labels Aug 25, 2021
@carpawell carpawell self-assigned this Aug 25, 2021
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #770 (84ecf90) into master (3ff2e31) will decrease coverage by 0.24%.
The diff coverage is 15.52%.

❗ Current head 84ecf90 differs from pull request most recent head 2458ac5. Consider uploading reports for the commit 2458ac5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
- Coverage   40.28%   40.03%   -0.25%     
==========================================
  Files         223      224       +1     
  Lines       10900    11013     +113     
==========================================
+ Hits         4391     4409      +18     
- Misses       6158     6253      +95     
  Partials      351      351              
Impacted Files Coverage Δ
cmd/neofs-node/config/cast.go 86.66% <0.00%> (-13.34%) ⬇️
pkg/innerring/processors/audit/processor.go 0.00% <0.00%> (ø)
pkg/innerring/processors/governance/processor.go 0.00% <0.00%> (ø)
pkg/innerring/processors/netmap/processor.go 0.00% <0.00%> (ø)
pkg/morph/client/notary.go 0.00% <0.00%> (ø)
pkg/morph/client/static.go 0.00% <0.00%> (ø)
pkg/morph/event/container/delete.go 60.86% <ø> (ø)
pkg/morph/event/container/put.go 50.00% <0.00%> (-3.85%) ⬇️
pkg/morph/event/container/put_notary.go 0.00% <0.00%> (ø)
pkg/network/address.go 87.23% <ø> (+3.56%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ff2e31...2458ac5. Read the comment docs.

@carpawell carpawell marked this pull request as ready for review August 25, 2021 15:28
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
`fallbackTime` is delta b/w `ValidUntilBlock` of
the main transaction and block when `fallback`
transaction is sent.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Refactor comments so they start with the names of
methods.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Prepare all listening structures for notary events:
rename(add prefix/suffix 'notification') all
notification specific handlers/parsers.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
}

// NotaryTypeFromBytes converts bytes slice to NotaryType.
func NotaryTypeFromBytes(data []byte) NotaryType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused, do we need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

did just the same as in notification.go(not used too). for future "potential" usage I guess

delete?


func initContainerService(c *cfg) {
wrap, err := wrapper.NewFromMorph(c.cfgMorph.client, c.cfgContainer.scriptHash, 0)
wrap, err := wrapper.NewFromMorph(c.cfgMorph.client, c.cfgContainer.scriptHash, 0, wrapper.TryNotary())
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if it breaks container.Delete and container.SetEACL

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it breaks

@carpawell carpawell requested a review from alexvanin September 5, 2021 21:39
@carpawell
Copy link
Member Author

carpawell commented Sep 5, 2021

@alexvanin, added resigning SN's notary request by IR instead of creating a new one

key changes:

  • notaryPreparator is struct now, not func. checking received requests is stricter now
  • added resigning notary request in morph/client.notary.go
  • added retrieving low-level morph client via Morph() method in wrappers
  • Put event contains a raw notary request pointer. Put handling logic in IR processor depends on that fact

Questions:

  • naming for IsAlphabet() option
  • time of contracts processors interface merging(in this PR or after all PR about notary flow notifications are merged)

@carpawell
Copy link
Member Author

@AnnaShaleva, can you, please, take a look at ce63109 and give us feedback about SN's notary request checking by alphabet node?


s.blockChan <- b
case response.NotaryRequestEventID:
notaryRequest, ok := notification.Value.(*response.NotaryRequestEvent)
Copy link
Member

Choose a reason for hiding this comment

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

You're probably not interested in removed notary requests, only in those that have been added to the pool. So you may filter out removed ones right here:

if notaryRequest.Type == mempoolevent.TransactionRemoved {
    continue
}

Copy link
Member

Choose a reason for hiding this comment

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

I see that removed requests are being filtered later, but anyway, do we need them at all?

Copy link
Member

Choose a reason for hiding this comment

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

To be fair that's the question I'm asking myself in general (subscription API-wise), who needs them?

Copy link
Member Author

@carpawell carpawell Sep 6, 2021

Choose a reason for hiding this comment

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

just wanted to make base functionality in node listener: it can listen to, parse and handle any notary requests

but yes, there are no parsers and handlers for such(removes notary requests) events in the alphabet node now

/cc @alexvanin

Copy link
Member

@AnnaShaleva AnnaShaleva Sep 6, 2021

Choose a reason for hiding this comment

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

To be fair that's the question I'm asking myself in general (subscription API-wise), who needs them?

I think it still can be a nice indicator of the case when neither main nor fallback transaction was completed. Other cases are covered by existing subscriptions API, e.g. subscriptions for transactions execution (main or fallback).

Copy link
Contributor

@alexvanin alexvanin left a comment

Choose a reason for hiding this comment

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

Let's rename IsAlphabet to AsAlphabet and call it a day for this PR.
In next PR I expect to see all container related notification with notary notification support, merged interfaces of NotaryContractProcessor and ContractProcessor and small processor event handling refactor.

}

func (p Preparator) validateExpiration(fbTX *transaction.Transaction) error {
if len(fbTX.Attributes) != 3 || fbTX.Attributes[1].Type != transaction.NotValidBeforeT {
Copy link
Member

@AnnaShaleva AnnaShaleva Sep 7, 2021

Choose a reason for hiding this comment

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

The attributes order is undefined in general, so here you check the way neo-go RPC client constructs them. I suggest to keep attributes length check and use fbTX.GetAttributes(transation.NotValidBefore) instead of fetching attribute by index.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, added more general attribute check

Add preparator for notary requests. Is parses
raw notary requests, checks if it should be
handled by Alphabet node. If handling is required,
returns `NotaryEvent` that contains information
about contract scripthash, method name and
arguments of the call.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Add handlers and parsers functionality for listener.
Separate notification and notary events by files.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Add access to low-level morph client in
wrappers

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell merged commit b4378f7 into nspcc-dev:master Sep 7, 2021
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
`fallbackTime` is delta b/w `ValidUntilBlock` of
the main transaction and block when `fallback`
transaction is sent.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Refactor comments so they start with the names of
methods.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Prepare all listening structures for notary events:
rename(add prefix/suffix 'notification') all
notification specific handlers/parsers.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Add preparator for notary requests. Is parses
raw notary requests, checks if it should be
handled by Alphabet node. If handling is required,
returns `NotaryEvent` that contains information
about contract scripthash, method name and
arguments of the call.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Add handlers and parsers functionality for listener.
Separate notification and notary events by files.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Add access to low-level morph client in
wrappers

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Add `NotaryInvokeNotAlpha` to low-level
client. It creates and sends notary request
that must be signed by Alphabet nodes, but
does not sign it by current node's private
key.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Add `NotarySignAndInvokeTX` method to morph
client. This function allows invoking notary
request with passed main TX(not creating a
new one). It signs passed main TX with
client's key.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell deleted the feature/reduce-container-creation-delay branch September 7, 2021 09:55
carpawell added a commit that referenced this pull request Sep 7, 2021
Implement `NotaryContractProcessor` by IR
container processor. Add support for notary
`put` container operation. Do not parse `put`
non-notary notifications in notary enabled
environment.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Storage Node needs to have notary deposit
for successful notary request sending.
Add notary deposit on startup(and wait for
its acceptance). Add notary deposit timer,
its config in `morph` section and env vars
for its tuning.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Sep 7, 2021
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improving existing functionality neofs-ir Inner Ring node application issues neofs-storage Storage node application issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce container creation delay

4 participants