Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

CBR-499: Extend LastBksSlots data type and storage for OBFT #4003

Merged
merged 5 commits into from
Jan 31, 2019
Merged

CBR-499: Extend LastBksSlots data type and storage for OBFT #4003

merged 5 commits into from
Jan 31, 2019

Conversation

erikd
Copy link
Member

@erikd erikd commented Jan 14, 2019

Description

Keep the hash of the public key used to sign each block for the last k blocks as required by the OBFT protocol.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-499

mhuesch
mhuesch suggested changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@mhuesch mhuesch left a comment

Choose a reason for hiding this comment

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

Looks great. I have some nitpicks and questions to check my understanding. I don't think that adoption of my suggestions would cause behavioral changes - they're more for clarity.

Q: why do we have an IORef in addition to the database state? Presumably anywhere we read an IORef we need IO, so then we could use the DB. Is this a caching/performance thing?

db/src/Pos/DB/Block/GState/BlockExtra.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Logic/Internal.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Logic/Util.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Logic.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Context.hs Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Logic.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Logic.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Logic.hs Outdated Show resolved Hide resolved
utxo/src/UTxO/Verify.hs Show resolved Hide resolved
@erikd
Copy link
Member Author

erikd commented Jan 15, 2019 •
edited

Q: why do we have an IORef in addition to the database state? Presumably anywhere we read an IORef we need IO, so then we could use the DB. Is this a caching/performance thing?

I think we have it in the DB and an IORef so we can read it off disk on startup, but use the IORef at run time. This is not the way I would have done it. If I was to do this from scratch, I would never keep this data in the database, only in memory and on startup, recreate this structure from the data base.

mhuesch
mhuesch previously approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@mhuesch mhuesch left a comment

Choose a reason for hiding this comment

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

Q: why do we have an IORef in addition to the database state? Presumably anywhere we read an IORef we need IO, so then we could use the DB. Is this a caching/performance thing?

I think we have it in the DB and an IORef so we can read it off disk on startup, but use the IORef at run time. This is not the way I would have done it. If I was to do this from scratch, I would never keep this data in the database, only in memory and on startup, recreate this structure from the data base.

Gotcha. The DB+IORef approach does seem like a weird design decision, and your suggestion makes much more sense. But this works, so LGTM :shipit: besides that mainnet syncing is failing.

db/src/Pos/DB/Block/GState/BlockExtra.hs Outdated Show resolved Hide resolved
db/src/Pos/DB/Block/Slog/Context.hs Show resolved Hide resolved
@erikd
Comments on SlotCount data type
f4e8801
@erikd
chain: Use medium hashes for improved debugging
cf40ab3
@erikd
generator: Only generate valid BlockScenarios
826e4b2 
The existing generator was generating 'BlockScenario's with
two blocks with the same hash in a sequence of applys. This
happened a number of times while I was debugging but now I
can't reproduce it without this patch. Will leave this patch
in just to make sure.
@erikd erikd force-pushed the erikd/CBR-499 branch 3 times, most recently from b49ab4a to 59eb3f7 Compare January 30, 2019 06:56
@erikd
Copy link
Member Author

erikd commented Jan 30, 2019

I tried changing it so that LastBlkSlots was only kept in an IORef but that would have required some really gnarly plumbing of Genesis.Config in the explorer code so I decided against it.

erikd
erikd commented Jan 30, 2019
View reviewed changes
db/src/Pos/DB/Block/Slog/Logic.hs
newLastSlots lastSlots = lastSlots & _Wrapped %~ updateLastSlots
dropEnd n xs = take (length xs - n) xs

lastSlotsToAppend :: [LastSlotInfo]
Copy link
Member Author

Choose a reason for hiding this comment

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

lastSlotsToAppend replaces lastSlotsToPrepend which was wrong because it failed to maintain the OldestFirst invariant.

@erikd erikd dismissed mhuesch’s stale review January 30, 2019 07:14

Dismissing review to guarantee a fresh review since the code has changed so much.

mhuesch
mhuesch approved these changes Jan 30, 2019
View reviewed changes
Copy link
Contributor

@mhuesch mhuesch left a comment

Choose a reason for hiding this comment

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

Nice work wrangling this crazy code :shipit:

db/src/Pos/DB/Block/GState/BlockExtra.hs Show resolved Hide resolved
db/src/Pos/DB/Block/GState/BlockExtra.hs Show resolved Hide resolved
@mhuesch
Copy link
Contributor

mhuesch commented Jan 30, 2019

bors r+

intricate
intricate approved these changes Jan 30, 2019
View reviewed changes
Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

LGTM!

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 30, 2019

Merge conflict (retrying...)

iohk-bors bot added a commit that referenced this pull request Jan 30, 2019
@iohk-bors @erikd @KtorZ
Merge #4003 #4040
b715928 
4003: CBR-499: Extend LastBksSlots data type and storage for OBFT r=mhuesch a=erikd

## Description

Keep the hash of the public key used to sign each block for the last `k` blocks as required by the OBFT protocol.

## Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-499


4040: Batch Import Addresses to 1.5.0 r=KtorZ a=KtorZ

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

Backporting cardano-foundation/cardano-wallet#259 to 1.5.0


## Linked issue

<!--- Put here the relevant issue from YouTrack -->



Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 30, 2019

Timed out (retrying...)

@disassembler
Copy link
Contributor

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 30, 2019

Canceled

@disassembler
Copy link
Contributor

bors r+ p=7

iohk-bors bot added a commit that referenced this pull request Jan 31, 2019
@iohk-bors @erikd
Merge #4003
f2099fb 
4003: CBR-499: Extend LastBksSlots data type and storage for OBFT r=disassembler a=erikd

## Description

Keep the hash of the public key used to sign each block for the last `k` blocks as required by the OBFT protocol.

## Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-499


Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 31, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit 8352b33 into develop Jan 31, 2019
@iohk-bors iohk-bors bot deleted the erikd/CBR-499 branch January 31, 2019 05:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhuesch mhuesch mhuesch approved these changes

@intricate intricate intricate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@erikd @mhuesch @disassembler @intricate