Ledger integration #585
Merged
Ledger integration #585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Nicholas Clarke <[email protected]> Co-authored-by: Thomas Winant <[email protected]>
edsko
commented
May 30, 2019
This keeps the complication of Signable where it belongs (in PBFT).
This is consistent with what we do elsewhere in the codebase, and cleans stuff up considerably.
Clean up these modules at the same time.
Main reason: get rid of `toCBORBlockWithoutBoundary`. Deal with the renaming of `StakeholderId` to `KeyHash`.
In particular, thread it through for the KES stuff, even if we still require it to be empty in 'leaves'.
nc6
approved these changes
May 30, 2019
| -- While the primitives in the ChainSync protocol are "roll back", "roll | ||
| -- forward (apply block)", etc. The /real/ primitive is "switch to | ||
| -- fork", which means that a roll back is always followed by applying at | ||
| -- least as many blocks that we rolled back. |
There was a problem hiding this comment.
This assumes that chain selection will always prefer a longer chain, which isn't necessarily the case?
There was a problem hiding this comment.
No, this assumes it will always prefer a chain that is at least as long as the current chain. That will be the case, right?
The `getCreator` method, which tells you which node produced a block, is used in the checks of some consensus tests. The `getCreator` method takes a `NodeConfig`, but we didn't have a `NodeConfig` in those testing contexts, so we were using `error` instead. This was "fine" because this `NodeConfig` is only really used for the real PBFT implementation, which is not used in the tests, only in the demo. However, using `error` here is dirty and might work for now, but that may change in the future. Fix this by letting the test setup function return the `NodeConfig` for each node in addition to its chain. This `NodeConfig` can then be passed to `getCreator`. Actually, `getCreator` would work with any `NodeConfig` from the test setup, regardless to which node its corresponds. This is because `getCreator` only reads data from the `NodeConfig` that is common to all nodes, no node-specific data is used. Nonetheless, it is nicer to use the `NodeConfig` that belongs to "correct" node.
mrBliss
approved these changes
May 30, 2019
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
May 30, 2019
585: Ledger integration r=mrBliss a=edsko Co-authored-by: Nicholas Clarke <[email protected]> Co-authored-by: Thomas Winant <[email protected]> Co-authored-by: Edsko de Vries <[email protected]> Co-authored-by: Thomas Winant <[email protected]>
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored-by: Nicholas Clarke [email protected]
Co-authored-by: Thomas Winant [email protected]