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

First exploration: Jörmungandr NetworkLayer #366

Closed
wants to merge 6 commits into from

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Jun 4, 2019

Issue Number

#219

Overview

Just exploring. WIP.

  • Change tip to (SlotId, BlockId). Should maybe have BlockHeader instead.
  • We need the genesis hash! (HasGenesisHash target or something)

Comments

  • The undefineds we have to pass in the tests are a bit unpleasant.
  • Maybe we don't want newtype BlockId

@Anviking Anviking self-assigned this Jun 4, 2019
@KtorZ
Copy link
Member

KtorZ commented Jun 5, 2019

Should maybe have BlockHeader instead.

Sounds like a good idea. Extend the BlockHeader type, and have BlockHeader instead.

@KtorZ
Copy link
Member

KtorZ commented Jun 5, 2019

We need the genesis hash! (HasGenesisHash target or something)

Why though ?

@Anviking
Copy link
Collaborator Author

Anviking commented Jun 5, 2019

Why though ?

initWallet = Wallet mempty mempty (SlotId 0 0, error "todo: genesisHash")

@KtorZ
Copy link
Member

KtorZ commented Jun 5, 2019

Good point. I guess we have our first configuration parameter then 😅

blocks `shouldBe` Right []

it "should work for the first epoch" $ do
Right blocks <- runExceptT $ nextBlocks network (SlotId 0 0)
Right blocks <- runExceptT $ nextBlocks network (SlotId 0 0, undefined)
Copy link
Member

Choose a reason for hiding this comment

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

I think the idea was more like.. to update the network layer to play nicely with this. Which implies also an update in the http bridge implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to have:

data NetworkLayer t m = NetworkLayer
    { nextBlocks :: BlockHeader -> ExceptT ErrNetworkUnreachable m [Block]
    , networkTip :: ExceptT ErrNetworkTip m BlockHeader

And these tests use the NetworkLayer, not the HttpBridgeLayer.

Without parametrising over tip, I don't see how we can avoid supplying undefined in place of a Hash "BlockHeader" here.

@Anviking
Copy link
Collaborator Author

Anviking commented Jun 5, 2019

🤔 Oops, it would be the parentHeaderHash of the genesis block, which at least for jormungandr is just \NULs.

Originally I believed we would have the BlockId / Hash "BlockHeader" of the current block instead.

@KtorZ
Copy link
Member

KtorZ commented Jun 5, 2019

Originally I believed we would have the BlockId / Hash "BlockHeader" of the current block instead.

Well, we don't have the "current" block header either when we initially instantiate the wallet. I wouldn't go for a Maybe here for it'll pollute everything else just because of the initialization. Still, it's probably a good idea to have the genesis hash somewhere available as a config param. This allows for at least checking that we are dealing with the blockchain we expect and that we agree on the initial config.

@Anviking
Copy link
Collaborator Author

Anviking commented Jun 7, 2019

Starting from a clean slate -> #384

@Anviking Anviking closed this Jun 7, 2019
@Anviking Anviking deleted the anviking/219/jörmungandr-network-layer branch June 7, 2019 17:49
@@ -461,34 +462,35 @@ newWalletLayer db nw tl = do
restoreSleep wid slot
Right blocks -> do
let next = view #slotId . header . last $ blocks
let nextBlock = BlockId $ view #prevBlockHash. header . last $ blocks -- TODO: WRONG!! THIS IS PARENT!!!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think nextBlocks nw should be responsible for returning a new cursor

@KtorZ KtorZ mentioned this pull request Jul 3, 2019
12 tasks
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.

None yet

2 participants