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

morph: use Notary Actor for notary requests submission #2310

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Apr 19, 2023

This PR is a tiny part of neo-go version upgrade.

And adjust the way how newly-created SN notary request is defined. Notary actor doesn't fill dummy witness for multisignature account (and that's the way how the system is supposed to function, see the documentation 7-th instruction at
https://github.com/nspcc-dev/neo-go/blob/master/docs/notary.md#2-request-submission). Thus, to check the request was just received from the storage node we must ensure the inner ring witness has empty invocation script.

That was the reason of nspcc-dev/neo-go#2955.

Refs. nspcc-dev/neo-go#2981.

TODO:

  • Need to resolve TODOs marked in code, @roman-khimov, need your decision.
  • Update CHANGELOG.

@AnnaShaleva
Copy link
Member Author

Checked on the dev-env against the nspcc-dev/neo-go#2955, SNs are able to properly enter the netmap:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neofs-node$ ./bin/neofs-cli netmap snapshot -g -r s01.neofs.devenv:8080
Epoch: 2
Node 1: 022bb4041c50d607ff871dec7e4cd7778388e0ea6849d84ccbd9aa8f32e16a8131 ONLINE /dns4/s01.neofs.devenv/tcp/8080 
	Continent: Europe
	Country: Russia
	CountryCode: RU
	Location: Moskva
	Price: 22
	SubDiv: Moskva
	SubDivCode: MOW
	UN-LOCODE: RU MOW
	User-Agent: NeoFS\/0.33
Node 2: 02ac920cd7df0b61b289072e6b946e2da4e1a31b9ab1c621bb475e30fa4ab102c3 ONLINE /dns4/s03.neofs.devenv/tcp/8080 
	Continent: Europe
	Country: Sweden
	CountryCode: SE
	Location: Stockholm
	Price: 11
	SubDiv: Stockholms l�n
	SubDivCode: AB
	UN-LOCODE: SE STO
	User-Agent: NeoFS\/0.33
Node 3: 038c862959e56b43e20f79187c4fe9e0bc7c8c66c1603e6cf0ec7f87ab6b08dc35 ONLINE /dns4/s04.neofs.devenv/tcp/8082/tls /dns4/s04.neofs.devenv/tcp/8080 
	Continent: Europe
	Country: Finland
	CountryCode: FI
	Location: Helsinki (Helsingfors)
	Price: 44
	SubDiv: Uusimaa
	SubDivCode: 18
	UN-LOCODE: FI HEL
	User-Agent: NeoFS\/0.33
Node 4: 03ff65b6ae79134a4dce9d0d39d3851e9bab4ee97abf86e81e1c5bbc50cd2826ae ONLINE /dns4/s02.neofs.devenv/tcp/8080 
	Continent: Europe
	Country: Russia
	CountryCode: RU
	Location: Saint Petersburg (ex Leningrad)
	Price: 33
	SubDiv: Sankt-Peterburg
	SubDivCode: SPE
	UN-LOCODE: RU LED
	User-Agent: NeoFS\/0.33

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #2310 (718c12b) into master (9831353) will increase coverage by 0.07%.
The diff coverage is 5.88%.

❗ Current head 718c12b differs from pull request most recent head d45e68c. Consider uploading reports for the commit d45e68c to get more accurate results

@@            Coverage Diff             @@
##           master    #2310      +/-   ##
==========================================
+ Coverage   29.94%   30.01%   +0.07%     
==========================================
  Files         400      400              
  Lines       30269    30205      -64     
==========================================
+ Hits         9063     9066       +3     
+ Misses      20467    20400      -67     
  Partials      739      739              
Impacted Files Coverage Δ
pkg/morph/client/notary.go 0.00% <0.00%> (ø)
pkg/morph/event/notary_preparator.go 76.33% <100.00%> (+0.42%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/morph/client/notary.go Outdated Show resolved Hide resolved
pkg/morph/client/notary.go Outdated Show resolved Hide resolved
}...)

if len(mainTx.Signers) > 3 {
// Invoker signature (simple signature account of storage node is expected).
Copy link
Member

Choose a reason for hiding this comment

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

Then a fake one works fine as well, we're only supposed to be adding IR signatures now (we don't have any other cases for now), so we don't care who is the third signer, a correct signature is supposed to be present there anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that to create a proper notary.FakeSimpleAccount(pub) we need to parse existing verification script to get the pub out of it. The same problem with multisig account. And I'd like to completely avoid parsing, it's just not needed here and this signer is unesed anyway.

pkg/morph/client/notary.go Outdated Show resolved Hide resolved
pkg/morph/client/notary.go Show resolved Hide resolved
pkg/morph/client/notary.go Show resolved Hide resolved
pkg/morph/client/notary.go Outdated Show resolved Hide resolved
roman-khimov
roman-khimov previously approved these changes Apr 21, 2023
@AnnaShaleva AnnaShaleva dismissed roman-khimov’s stale review April 21, 2023 07:47

Backwards compatibility code is added to the notary preparator.

And adjust the way how newly-created SN notary request is defined.
Notary actor doesn't fill dummy witness for multisignature account
(and that's the way how the system is supposed to function, see
the documentation 7-th instruction at
https://github.com/nspcc-dev/neo-go/blob/master/docs/notary.md#2-request-submission).
Thus, to check the request was just received from the storage node
we must ensure the inner ring witness has empty invocation script.

That was the reason of nspcc-dev/neo-go#2955.

Signed-off-by: Anna Shaleva <anna@nspcc.ru>
It's unused and not needed, default fallback lifetime is set by Notary
actor.

Signed-off-by: Anna Shaleva <anna@nspcc.ru>
@roman-khimov roman-khimov merged commit 4e75e84 into master Apr 21, 2023
@roman-khimov roman-khimov deleted the use-notarizator branch April 21, 2023 12:02
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.

2 participants