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.

Sign up for GitHub

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

Jump to bottom

Allow multiple credentials #2640

Merged
merged 4 commits into from
Oct 1, 2020
Merged

Allow multiple credentials #2640

merged 4 commits into from
Oct 1, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Sep 28, 2020

For each set of credentials, a separate block forging thread is spawned.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Sep 28, 2020
@mrBliss mrBliss requested a review from edsko September 28, 2020 16:04
mrBliss
mrBliss commented Sep 28, 2020
View reviewed changes
Copy link
Contributor Author

@mrBliss mrBliss left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TODOs:

  • It would nice if the trace messages included information about the BlockForging that traced them. I was thinking of adding an extra field to BlockForging (Text? Or a new type family? In practice the hash of the issuer key or whatever could be used).

  • In the HFC, we just "zip" them, which means that if we have 1 Byron cred and 3 Shelley creds, we'd have three threads: one with OptCons Byron1 $ OptCons Shelley1 $ OptNil and two with OptSkip $ OptCons Shelley2/3 $ OptNil.

UPDATE: done

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs
@@ -146,7 +145,7 @@ initNodeKernel args@NodeArgs { registry, cfg, tracers, maxTxCapacityOverride

st <- initInternalState args

whenJust blockForging $ forkBlockForging maxTxCapacityOverride st
mapM_ (forkBlockForging maxTxCapacityOverride st) blockForging
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main change: we spawn a separate thread for each BlockForging. This means each thread will do the same checks, including ticking the chain, which can be expensive.

ouroboros-consensus/ouroboros-consensus.cabal Outdated Show resolved Hide resolved
mrBliss
mrBliss commented Sep 29, 2020
View reviewed changes
Copy link
Contributor Author

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

It would nice if the trace messages included information about the BlockForging that traced them. I was thinking of adding an extra field to BlockForging (Text? Or a new type family? In practice the hash of the issuer key or whatever could be used).

Add a field, include it in the Byron and Shelley credentials.

ouroboros-consensus-byron-test/src/Ouroboros/Consensus/ByronDual/Node.hs Outdated Show resolved Hide resolved
ouroboros-consensus-cardano/src/Ouroboros/Consensus/Cardano/Node.hs Outdated Show resolved Hide resolved
@mrBliss mrBliss force-pushed the mrBliss/multiple-credentials branch 3 times, most recently from 7e30aac to 9fe83d9 Compare September 29, 2020 12:23
@mrBliss mrBliss marked this pull request as ready for review September 29, 2020 12:23
mrBliss
mrBliss commented Sep 30, 2020
View reviewed changes
ouroboros-consensus-byron/src/Ouroboros/Consensus/Byron/Node.hs Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/TPraos.hs Outdated Show resolved Hide resolved
ouroboros-consensus-test/test-consensus/Test/Consensus/HardFork/Combinator.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/AcrossEras.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Protocol.hs Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Forging.hs Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Forging.hs Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Forging.hs Show resolved Hide resolved
edsko
edsko approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Nice, this is so much better.

@mrBliss mrBliss force-pushed the mrBliss/multiple-credentials branch 2 times, most recently from ef8cee5 to dd70adb Compare September 30, 2020 09:29
@mrBliss
Copy link
Contributor Author

mrBliss commented Sep 30, 2020

(I'm waiting until the release has been tagged before merging this).

mrBliss and others added 4 commits October 1, 2020 16:42
@mrBliss @deepfire
Allow multiple credentials
ab3f2b8 
For each set of credentials, a separate block forging thread is spawned.

Co-authored-by: Kosyrev Serge <serge.kosyrev@iohk.io>
@mrBliss
Label forge-related trace messages with TraceLabelCreds
751d5b6 
The Byron and Shelley credentials now include a label to be used as the
`forgeLabel` field of `BlockForging`.

This label will be included in the forge-related trace messages so that it is
possible to see which trace messages belong to which set of credentials, when
running a node with multiple sets of credentials.

In practice, one could use a hash of the public key used to sign
blocks (displayed in base 16).
@mrBliss
AcrossEras: sort the declarations alphabetically
4a1fffd
@mrBliss
Remove left-over Ouroboros.Consensus.HardFork.Combinator.Forge
6f11d25 
This module was dead code that got replaced by
`Ouroboros.Consensus.HardFork.Combinator.Forging`, but we forgot to remove the
file itself.
@mrBliss mrBliss force-pushed the mrBliss/multiple-credentials branch from dd70adb to 6f11d25 Compare October 1, 2020 15:03
@mrBliss
Copy link
Contributor Author

mrBliss commented Oct 1, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 1, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit cc21918 into master Oct 1, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/multiple-credentials branch October 1, 2020 15:24
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Oct 6, 2020
@mrBliss
Update dependency on ouroboros-network
c831ae5 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
@mrBliss mrBliss mentioned this pull request Oct 6, 2020
Closed
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 6, 2020
@mrBliss @coot
Update dependency on ouroboros-network
263992d 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
deepfire pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 6, 2020
@mrBliss @deepfire
Update dependency on ouroboros-network
882178a 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 7, 2020
@mrBliss @coot
Update dependency on ouroboros-network
806e974 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 7, 2020
@mrBliss @coot
Update dependency on ouroboros-network
6c663f4 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 7, 2020
@mrBliss @coot
Update dependency on ouroboros-network
f1f7b48 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
newhoggy pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 9, 2020
@mrBliss @newhoggy
Update dependency on ouroboros-network
1d0ae7d 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
newhoggy pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 9, 2020
@mrBliss @newhoggy
Update dependency on ouroboros-network
7548f98 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Oct 9, 2020
@mrBliss
Update dependency on ouroboros-network
3ffa6c6 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
newhoggy pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 12, 2020
@mrBliss @newhoggy
Update dependency on ouroboros-network
e39c8e0 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 12, 2020
@mrBliss @coot
Update dependency on ouroboros-network
92f2cf9 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Oct 13, 2020
@mrBliss @coot
Update dependency on ouroboros-network
87db7d1 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
JaredCorduan pushed a commit to IntersectMBO/cardano-node that referenced this pull request Nov 3, 2020
@mrBliss @JaredCorduan
Update dependency on ouroboros-network
85da83a 
Notable changes:
* IntersectMBO/ouroboros-network#2640
* IntersectMBO/ouroboros-network#2649
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepfire deepfire deepfire left review comments

@edsko edsko edsko approved these changes

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrBliss @deepfire @edsko