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.

Sign up for GitHub

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

Jump to bottom

network: add notary request payload #1582

Merged
merged 4 commits into from
Dec 10, 2020
Merged

network: add notary request payload #1582

merged 4 commits into from
Dec 10, 2020

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Dec 1, 2020 •
edited
Loading

Close #1546.

@AnnaShaleva AnnaShaleva marked this pull request as draft December 1, 2020 10:55
@codecov
Copy link

codecov bot commented Dec 1, 2020 •
edited
Loading

Codecov Report

Merging #1582 (0b5cf78) into master (f0dba26) will increase coverage by 0.00%.
The diff coverage is 83.01%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1582    +/-   ##
========================================
  Coverage   82.39%   82.40%            
========================================
  Files         238      240     +2     
  Lines       18833    19052   +219     
========================================
+ Hits        15518    15700   +182     
- Misses       2322     2356    +34     
- Partials      993      996     +3
Impacted Files Coverage Δ
pkg/network/server.go 73.92% <50.00%> (-2.17%) ⬇️
pkg/network/notary_feer.go 83.33% <83.33%> (ø)
pkg/core/blockchain.go 76.00% <84.74%> (+0.01%) ⬆️
pkg/network/payload/notary_request.go 87.50% <87.50%> (ø)
pkg/core/native/policy.go 82.88% <88.88%> (+3.54%) ⬆️
pkg/core/native/notary.go 86.38% <90.00%> (+0.15%) ⬆️
pkg/core/native/util.go 87.87% <90.00%> (+3.26%) ⬆️
pkg/consensus/consensus.go 67.85% <100.00%> (ø)
pkg/core/mempool/mem_pool.go 96.58% <100.00%> (+0.31%) ⬆️
pkg/network/message.go 95.95% <100.00%> (+0.08%) ⬆️
... 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 f0dba26...0b5cf78. Read the comment docs.

@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 3 times, most recently from 033af91 to 8b459e0 Compare December 1, 2020 15:51
roman-khimov
roman-khimov requested changes Dec 1, 2020
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

One thing really worrying me here is tying request pool to the Blockchain. I'd really like to keep it in the network.Server somewhere.

Another big topic is separation of concerns between payload/pool/service checks. On one hand, I'd like to minimize the burden on non-service nodes (theoretically we could allow exchanging abstract blobs across the network just checking for the signature and deposit), on the other broadcasting invalid (to be rejected by the service) requests doesn't help the network either.

fyrchik
fyrchik reviewed Dec 2, 2020
View reviewed changes
pkg/core/blockchain.go
@@ -934,6 +947,11 @@ func (bc *Blockchain) GetGoverningTokenBalance(acc util.Uint160) (*big.Int, uint
return &neo.Balance, neo.LastUpdatedBlock
}

// GetDepositFor returns GAS amount deposited to Notary contract for the specified account.
func (bc *Blockchain) GetDepositFor(acc util.Uint160) *big.Int {
return bc.contracts.Notary.BalanceOf(bc.dao, acc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see how dependency on native contracts may prevent this from being done outside of core. May be we can do something like what was done in #1427.
E.g. updating GetBalance() function during OnPersistEnd().

Copy link
Member Author

Choose a reason for hiding this comment

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

We have (*NotaryPool).PostBlock now, which can make use of Blockchainer. Do we still need this updating function?

@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 4 times, most recently from 0aa3339 to c183fd4 Compare December 2, 2020 16:52
roman-khimov
roman-khimov reviewed Dec 2, 2020
View reviewed changes
@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 4 times, most recently from 3f2c483 to 3bf557b Compare December 3, 2020 16:06
roman-khimov
roman-khimov requested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks good, VerifyAndPoolPayload for state-dependent things and isValid for state-independent. All the logic is in place and just needs to be moved a little.

@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 3 times, most recently from 016c56f to 9cbf6a0 Compare December 4, 2020 12:32
@AnnaShaleva AnnaShaleva marked this pull request as ready for review December 4, 2020 12:34
@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 3 times, most recently from 7e88649 to 71abc3d Compare December 4, 2020 15:45
@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 10 times, most recently from 404b5d8 to 4631f7d Compare December 9, 2020 15:24
roman-khimov
roman-khimov reviewed Dec 9, 2020
View reviewed changes
@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 2 times, most recently from 1858dd9 to a69f9a7 Compare December 10, 2020 09:10
roman-khimov
roman-khimov reviewed Dec 10, 2020
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks good.

@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch 8 times, most recently from 6b48a8a to e8245a5 Compare December 10, 2020 15:08
@AnnaShaleva
core: check stack length before returning false verification result
2ab0e6c 
We must be sure that stack has no other items before returning `false`
verification result. It is an error in both cases, but by preserving the
order we know exactly that it was correct `false` on stack.
@AnnaShaleva
core: return a special error from verifyHashAgainstScript
6d357c3 
It will help us to distinguish proper `false` verification result from
various verification errors.
@AnnaShaleva
core: take into account NotaryAssisted attributes during verification
501c0c9 
It's a bug, we have to reserve proper amount of GAS from verification
gas limit for NotaryAssisted attributes.
@AnnaShaleva
network: add notary request payload
0b5cf78
@AnnaShaleva AnnaShaleva force-pushed the signature_collection/notary_request_payload branch from e8245a5 to 0b5cf78 Compare December 10, 2020 15:17
@roman-khimov roman-khimov merged commit 3bbf764 into master Dec 10, 2020
@roman-khimov roman-khimov deleted the signature_collection/notary_request_payload branch December 10, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@fyrchik fyrchik Awaiting requested review from fyrchik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P2P Notary request payload
3 participants
@AnnaShaleva @fyrchik @roman-khimov