Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Fix #307 #446

Merged
merged 1 commit into from Sep 28, 2018
Merged

Fix #307 #446

merged 1 commit into from Sep 28, 2018

Conversation

melanke
Copy link
Contributor

@melanke melanke commented Sep 26, 2018

No description provided.

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #446 into development will increase coverage by 0.1%.
The diff coverage is 74.07%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development     #446     +/-   ##
==============================================
+ Coverage        53.95%   54.06%   +0.1%     
==============================================
  Files              296      297      +1     
  Lines            12224    12262     +38     
==============================================
+ Hits              6596     6629     +33     
- Misses            5628     5633      +5
Impacted Files Coverage Δ
...arp.Core/Messaging/Handlers/BlockMessageHandler.cs 0% <0%> (ø) ⬆️
src/NeoSharp.Core/Network/Tcp/TcpPeer.cs 0% <0%> (ø) ⬆️
...Sharp.Core/Models/OperationManger/BlockVerifier.cs 100% <100%> (ø)
...Sharp.Core/Blockchain/Processing/BlockProcessor.cs 100% <100%> (ø) ⬆️
src/NeoSharp.Core/Blockchain/Genesis.cs 100% <100%> (ø) ⬆️
src/NeoSharp.Core/DI/Modules/BlockchainModule.cs 100% <100%> (ø) ⬆️
...eoSharp.Core/Models/OperationManger/BlockSigner.cs 100% <100%> (ø)

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 014f22a...283fe92. Read the comment docs.

this._blockSigner.Sign(block);
}

if (_blockVerifier.Verify(block))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can save a bit here - no need in verification if we just signed a block.

Copy link
Member

Choose a reason for hiding this comment

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

Will be verified before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant if the hash was null on line 58 and we signed on line 59. It makes a little sense to verify on line 62.

@osmirnov osmirnov merged commit 33c468d into CityOfZion:development Sep 28, 2018
@aboimpinto
Copy link
Contributor

In my PR I put everything back to the OperationManager
we should not have two classes for the same concern. The Signature and the Verification should be in the same place because most of the cases they share the same dependencies.

The solution for the Circular References is not splitting this functionalities but remove the IBlockchain and IServer from the MessageHandlers and I did that on my pending PR.

I've been working on this for the last 2 days as I said in Discord

image

I already revert the changes for the transactions on my pending PR

@lock9 lock9 deleted the verify_block branch October 29, 2018 14:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants