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

UnitTests working for Akka ConsensusService #475

Merged
merged 52 commits into from
Dec 4, 2018

Conversation

igormcoelho
Copy link
Contributor

@igormcoelho igormcoelho commented Nov 24, 2018

This PR is not meant to be directly accepted, my intention is just to discuss how we could efficiently port Unit Tests for Consensus code (perhaps in several PR, what is best to the community).
This sample here uses official Akka TestKit, and it is already capable of starting consensus in a given time period (01/01/1968 00:00:00), simulating a specific block height (=2), an internal state (index=2 changeview=0), and making it propose an specific block header. The PrepareRequest message was successfully received.
An example of the output (consensus logs are also captured in unit tests):

 CONSENSUS LOG: OnStart
 CONSENSUS LOG: initialize: height=2 view=0 index=2 role=Primary
 OnTimer should expire! 
 Waiting for subscriber message!
 CONSENSUS LOG: timeout: height=2 view=0 state=Primary
 CONSENSUS LOG: send prepare request: height=2 view=0
 MESSAGE 1: Neo.Network.P2P.LocalNode+SendDirectly
 CONSENSUS LOG: OnStop

I had to do some changes, I'll highlight the most fundamental ones:

  • allowing neo.UnitTests to see internal classes from neo (see Properties/AssemblyInfo.cs)
  • creating an interface for ConsensusContext (called IConsensusContext), that allows us to Mock all of its methods and simulate the desired results without interfering in ConsensusService
  • passing LocalNode and TaskManager actors directly to ConsensusService (instead of NeoSystem). I had to do this because on testing we use specially crafted TestActors to replace them
  • making Snapshot a private variable, thus only exposing its important properties (Height and previous Header)
  • allowing time manipulation via method
  • allowing log maniputation via method

Please take a look @erikzhang , @shargon, @vncoelho, @PeterLinX , I believe we can advance much faster in consensus issues by having a tool like this.

@shargon
Copy link
Member

shargon commented Nov 25, 2018

I think that unit testing on consensus is completely necessary! good job @igormcoelho

@igormcoelho
Copy link
Contributor Author

I agree Erik, moved TimeProvider to Neo.Time namespace, so timing can be easily shared (and tested) for the whole project.

public Dictionary<UInt256, Transaction> Transactions;
public byte[][] Signatures;
public byte[] ExpectedView;
public DateTime block_received_time { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have TimeProvider, can we move block_received_time back to ConsensusService? I did not see this variable used in ConsensusContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I lost my review, sorry for the delay Erik. In fact, we need manipulate block_received_time on unit tests... but one point I think now is that, could we use PrevHeader.Timestamp to replace block_received_time? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

You can't use PrevHeader.Timestamp because the system time of different nodes may be different.

You can control block_received_time through TimeProvider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you Erik, one less thing to control. Moving it back to Service.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Dec 2, 2018

What do you think @erikzhang? I guess we have agreed in most points now.

One thing we haven't discussed is the Log system. I moved it to ConsensusContext, what is not ideal in my opinion. I'm thinking of moving it back (since I'm not using this info right now), so we can minimize the changes and try to pass this to master. What do you think?

As soon as we have this on master, we can start creating tests to many interesting scenarios, including the fork situation.


public ConsensusContext(Wallet wallet)
{
this.wallet = wallet;
}

public bool RejectTx(Transaction tx, bool verify)
Copy link
Member

Choose a reason for hiding this comment

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

This section needs to be moved back to ConsensusService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Now I think I can do it without breaking encapsulation.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Dec 3, 2018

Ok @erikzhang, moved RejectTx logic back to ConsensusService and also the Log function.

The currently failing test is now depending only on this small fix: #498

@igormcoelho
Copy link
Contributor Author

Ok, back to normality, tests are running fine.

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.

4 participants