Venus: go-fakecoin should(?) be moved to its own repo

[frrist]

Description

This will allow developers to move quickly when implementing breaking features, and allows go-fakecoin can be fixed at a later date.

[dignifiedquire]

I am worried that we will just not update it anymore, until we want to make a demo, have issues if this happens.

[acruikshank]

The fakecoin tests are the biggest integration test we have in the project. I've found some pretty subtle bugs this way.

[frrist]

Once https://github.com/filecoin-project/go-filecoin/pull/659 merges the chain generated by go-fakecoin will be invalid, none of the messages in the blocks are signed, and they cannont be singed until go-fakecoin is updated to use addresses derived from keys. (gut check says this isn't a simple fix)

This issues was created while writing https://github.com/filecoin-project/go-filecoin/pull/659, I need to get it merged and this is a blocker of it. Fixing go-fakecoin felt like it was out of the scope of that PR (maybe i'm wrong in my thinking here). These are the possible solutions I see, with the goal being "Merge #659":

1. Fix it later:
   
   
   
   • Remove go-fakecoin and add back at a later date when we are ready to fix it.
   
   
   • Move go-fakecoin to a different repo and fix it at a later date, also setup CI for it, and create issues for it.
   
   
   • Leave go-fakecoin but don't run tests against it. Fix it at a later date.
   
   
2. Fix it now:
   
   
   
   • Leave go-fakecoin and freeze the go-filecoin code until it is fixed.
   
   
   • ~Fix go-fakecoin in #659 (please no :hankey: :sob: :radioactive: :crying_cat_face:)~
   
   

My vote is we remove it from the build/test process and fix it asap.

[porcuquine]

I think we should completely separate the issues:

1. Should go-fakecoin be its own project in its own repo?
2. Should the need to update go-fakecoin block some short-term progress?

It sounds like the answer to 2) might be 'no', and that's why we are considering 1). If that is the only reason 1) is on the table, I suggest we remove it from consideration. If there exist other reasons, those can be put forth and discussed on their own merits.

On that basis, my expedient suggestion would be to decouple the go-fakecoin tests. Maybe we can create a flag or test-group, or whatever, such that when in an explicitly-visible 'expedient' phase of the development cycle, failure of go-fakecoin tests won't block CI.

I think this corresponds to the third option under @frrist's 1) above: 'Leave go-fakecoin but don't run tests against it. Fix it at a later date.'

This seems to be sensible from first principles, but to adhere to those principles, I think we should at least put in place a mechanism for temporarily ignoring a category of tests which are part of a more rigorous/complete test suite. This issue will probably come up again, and it would be better to have a sane mechanism for dealing with it.

I think it would set a bad precedent if we start removing/disabling/nerfing tests or other functionality just because its presence is inconvenient. Formally acknowledging that there are tiers of tests and not all tiers need to be satisfied for expedient development seems like a reasonable strategy. That also implies that real resources will need to eventually be allocated to fixing.

If there are independent reasons to want to get rid of go-fakecoin, that's a completely separate issue. Based on @acruikshank's feedback above, it seems like we should be biased toward assuming long-term removal is a bad idea.

[frrist]

I have changes in the spirit of "Leave go-fakecoin but don't run tests against it. Fix it at a later date." here - https://github.com/filecoin-project/go-filecoin/pull/659/commits/339b9461ccb168b5cf77cc558522f988946c2beb#diff-0ffcfcadd6898daf8fa118ceeb59b3a7R27. Let's use that commit as a place for feedback. If we approve the changes made there I will create follow on issues for go-fakecoin.

go-fakecoin follow on issue here -https://github.com/filecoin-project/go-filecoin/issues/698

[acruikshank]

I would be fine dropping fakecoin or exporting fakecoin if we had a test (possibly a command test) that did roughly the same thing. That should be able to use the new signing logic.

[frrist]

@acruikshank what test(s) are you referring to in go-fakecoin?

[acruikshank]

@ffrist TestAddActors and TestAddFakeChain in tools/go-fakecoin/main_test.

[frrist]

Humm, I think I may be missing something then.

• TestAddFakeChain basically tests that the methods we pass to fake are called an appropriate number of times - this logic is more or less duplicated here. I don't see this as a critical test.
• TestAddActors tests that fakeActors creates new actors.
  
  
  
  • fakeActors creates some messages (createMiner, addAsk, addBid, addDeal) adds them to the message pool, mines to get them into blocks, and returns.
  
  
  • This logic is already tested elsewhere, although all 4 of the message types are not done in the same test. I don't see this as a critical test either.
  
  

These tests are important to maintaining the functionality of fakecoin, but I don't see what they are doing wrt to filecoin that our tests don't already cover.

This brings us to another question, what _should_ fakecoin be used for today, and what problems are we trying to solve with it?

[phritz]

Value:

• tool to create chains for us that are valid, eg for genesis
• block explorer
• tests -- unclear the value of

Status: currently broken, will take a ~week to fix

Conclusion: has value, needs to be fixed

[frrist]

closing in favor of https://github.com/filecoin-project/go-filecoin/issues/698