Skip to content

consensus/babe, core: read BABE authority updates from block consensus digests#595

Merged
noot merged 15 commits intodevelopmentfrom
noot/auth-data
Feb 11, 2020
Merged

consensus/babe, core: read BABE authority updates from block consensus digests#595
noot merged 15 commits intodevelopmentfrom
noot/auth-data

Conversation

@noot
Copy link
Copy Markdown
Contributor

@noot noot commented Feb 6, 2020

Changes

  • add NextEpochDescriptor which is what's put in the consensus digest at the first block of each epoch
  • add helper functions for AuthorityData
  • add Grandpa_authorities runtime call to retrieve authorities initially
  • add handleConsensusDigest function which is called after a block is imported to see if there is a NextEpochDescriptor

Tests:

go test ./consensus/... ./core/...

Issues:

closes #575

Copy link
Copy Markdown
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Lookin' good. A few questions and comments.

Comment thread consensus/babe/types.go
Comment thread core/runtime.go
Comment thread consensus/babe/types.go
Comment thread core/service.go
}

// TODO: our authority index should be in authData, if it isn't, we aren't authorities
// need to add our authority data to storage
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have an issue open to address this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll open one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread core/service.go
}

func (s *Service) retrieveAuthorityData() ([]*babe.AuthorityData, error) {
// TODO: when we update to a new runtime, will need to pass in the latest block number
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we should link these to an issue and update that runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will add to comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#563 should cover everything

Comment thread core/service.go
}
}

// TODO: if this block is the first in the epoch and it doesn't have a consensus digest, this is an error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not return an error? Not sure I understand the todo item here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we don't have a way to check what slot in the epoch we're at from core, I can open an issue for it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@edwardmack edwardmack left a comment

Choose a reason for hiding this comment

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

Look great, I see some TODOs regarding newer runtime, do you know if there are a lot of differences between runtimes? Are the newer runtimes changing interfaces a lot?

@noot
Copy link
Copy Markdown
Contributor Author

noot commented Feb 7, 2020

@edwardmack it seems like the main change from the runtime we have to the current runtime is the length of the randomness, otherwise, it appears to be the same

Copy link
Copy Markdown
Contributor

@ryanchristo ryanchristo 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 to me. Nice work! If you feel inspired and we want to try a new format, maybe adding the issue numbers to todos that are being introduced in this pull request. Not necessary though.

@noot noot merged commit 3417177 into development Feb 11, 2020
@noot noot deleted the noot/auth-data branch February 11, 2020 03:39
ryanchristo pushed a commit that referenced this pull request Jun 24, 2020
…s digests (#595)

* add NextEpochDescriptor which is what's put in the consensus digest at the first block of each epoch
* add helper functions for AuthorityData
* add Grandpa_authorities runtime call to retrieve authorities initially
* add handleConsensusDigest function which is called after a block is imported to see if there is a NextEpochDescriptor
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.

BABE: retrieve and set authority data

3 participants