fix/Notary request tracking

[carpawell]

Closes #2315

[codecov]

Codecov Report

Merging #2459 (e361c3b) into master (12e8b08) will increase coverage by 0.03%.
The diff coverage is 87.50%.

❗ Current head e361c3b differs from pull request most recent head 4fd9775. Consider uploading reports for the commit 4fd9775 to get more accurate results

@@            Coverage Diff             @@
##           master    #2459      +/-   ##
==========================================
+ Coverage   29.44%   29.47%   +0.03%     
==========================================
  Files         399      399              
  Lines       30410    30432      +22     
==========================================
+ Hits         8954     8971      +17     
- Misses      20713    20717       +4     
- Partials      743      744       +1     

Files Changed	Coverage Δ
pkg/morph/event/listener.go	0.00% <0.00%> (ø)
pkg/morph/event/parsers.go	0.00% <ø> (ø)
pkg/morph/event/utils.go	0.00% <ø> (ø)
pkg/morph/event/notary_preparator.go	77.89% <93.33%> (+1.56%)	⬆️

... and 1 file with indirect coverage changes

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

[roman-khimov]

Does it really close #2315? How about someone (an IR node) sending an epoch update prematurely?

[carpawell]

@roman-khimov, TBH, yesterday I figured out that it was not a problem at all. Yes, we parse every notary request and are ready to sign each of them but not every method has a handler and a parser for it: e.g. our beloved NewEpoch event is listened as a simple notification cause netmap processor has a parser and a handler for it. It is ready to update its internal state and be sure that everyone thinks that a NewEpoch has happened cause it believes the contract. But if another IR sends notary requests it won't be handled cause IR does not have NewEpoch notary parser and notary handler for it.

But why have I added the last commit then? I still think that morph refactor is somewhere near and we will drop (simplify) Listener. Also, I think notary events are missing contract side checks (the netmap contract will not allow ticking epoch if there are less than 5 IR requested a new epoch; but IR would tick epoch if there are a hander for it and a corresponding notary request has been parsed) so I think it should be a notary preparator (preparator now) who must skip unnecessary requests not a missing handler.

[AnnaShaleva]

Otherwise LGTM.

[AnnaShaleva]

We've discussed and came to the solution that transctions cache isn't needed and can be removed. The only purpose of this cache is to filter out notary requests that were sent by the alphabet node itself. Instead of this we can add a check for notary request: the second signer of fallback transaction shouldn't be the committee member.

Disadvantage of this approach is that we have to always keep the list of committee members up-to-date. It can be reached with RPC-based cache (but then a lot of getcommittee RPC requests are expected) or with cache that is subscribed to the committee updates (we need to add some code to our cache than).

@roman-khimov, what do you think?

[roman-khimov]

Do you mean alreadyHandledTXs?

sent by the alphabet node itself
the second signer of fallback transaction shouldn't be the committee member

So is it a node key that is a problem of is it a whole committee that is a problem? What if SN uses the same key as some IR node?

Hi, neo-project/neo#2763, anyway.

[AnnaShaleva]

Do you mean alreadyHandledTXs?

Yes.

What if SN uses the same key as some IR node?

But is it possible? I thought that SN and IR are always different antities with different accounts. If not, then the described way isn't workable.

[roman-khimov]

Nothing technically prevents SN from using the same key as IR.