CBR-525: Fix failure to sync issue when switching to OBFT #4153

erikd · 2019-05-30T07:34:11Z

Description

Fix issue that caused the node to loose sync with the chain in OBFT era and then fail to re-sync once out of sync.

Linked issue

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

CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Synced the OBFT testnet numerous times.

mhuesch · 2019-05-30T17:18:04Z

It seems that Hydra ran out of disk space when running over Erik's last push (source):

/nix/store/w9avgn80c1vqsn9bpm1x5401qknlc966-cctools-binutils-darwin/bin/ranlib: file: dist/build/libHScardano-sl-utxo-3.0.1-KHxprOrWJ7jBRIg17UkM2t-ghc8.4.4.a(elf_reloc_aarch64.o) has no symbols
fatal error: /nix/store/w9avgn80c1vqsn9bpm1x5401qknlc966-cctools-binutils-darwin/bin/ranlib: can't write to output file (No space left on device)
running auto-GC to free 26179796992 bytes
finding garbage collector roots...
`ranlib' failed in phase `Ranlib'. (Exit code: 1)

Since it ran a gc I thought it may work if I just retrigger CI, so I ran git commit --amend and force pushed.

There are also errors with AppVeyor (1 & 2), but they are not required so at present I plan to ignore them.

intricate

Just some comments about typos (no big deal). Going to run this on OBFT testnet and make sure it all goes well before approving on my end.

CHANGELOG.md

db/src/Pos/DB/Block/Logic/SplitByEpoch.hs

db/src/Pos/DB/Block/Logic/VAR.hs

db/src/Pos/DB/Block/Logic/SplitByEpoch.hs

mhuesch

Nice job. I left some comments. Approving because I do not believe any of the few issues I raised to be dealbreakers. The most important, I think, is this one but I expect that in practice the two values are always equal.

chain/src/Pos/Chain/Block/Slog/LastBlkSlots.hs

mhuesch · 2019-05-30T17:37:19Z

chain/src/Pos/Chain/Block/Logic/Integrity.hs

-        obftLeaderCanMint :: AddressHash PublicKey -> BlockCount -> OldestFirst [] LastSlotInfo -> Bool
-        obftLeaderCanMint leaderAddrHash blkSecurityParam (OldestFirst lastBlkSlots) =
+        obftLeaderCanMint :: AddressHash PublicKey -> BlockCount -> LastBlkSlots -> Bool
+        obftLeaderCanMint leaderAddrHash blkSecurityParam lastBlkSlots =
            blocksMintedByLeaderInLastKSlots leaderAddrHash lastBlkSlots
                <= leaderMintThreshold blkSecurityParam


Isn't there risk of a mismatch between blkSecurityParam and the lbsCount inside of lastBlkSlots? I think, but am not sure, that if those get out of sync we get an invalid comparison.

The LastBlkSlots is binning based on one k value, and the threshold demanded is based on another k value.

Perhaps we can just check that the values are equal at some point. They should always be equal, right?

if those get out of sync we get an invalid comparison.

The only way they could get out of sync was there was a successful update proposal to change it and I really don't think there will be such a thing.

There is however another related issue. For OBFT we are checking that the block we are validating has not been signed too often in the last k blocks when I think it should actually be last k slots. This is only the same thing if there are no slots with missing blocks.
Confirmed with @dcoutts that blocks is indeed correct.

db/src/Pos/DB/Block/Slog/Logic.hs

db/src/Pos/DB/Update/Logic/Global.hs

db/test/Test/Pos/DB/Block/Logic/SplitByEpoch.hs

chain/test/Test/Pos/Chain/Block/Slog/LastBlkSlots.hs

db/src/Pos/DB/Block/Logic/VAR.hs

mhuesch · 2019-05-30T20:01:50Z

I documented the delegator/delegatee story in this commit (also my auto-formatter apparently moved imports). I believe we are doing the right thing. Would appreciate a double-check from @intricate and @erikd.

chain/src/Pos/Chain/Block/Header.hs

db/src/Pos/DB/Block/Logic/VAR.hs

chain/src/Pos/Chain/Block/Header.hs

With property tests.

Co-authored-by: Luke Nadur <luke.nadur@iohk.io>

The old version of the struct was a list which made calculating the OBFT parameters highly inefficient. New version is much more efficient and has tests.

I also take the liberty of fixing a typo.

disassembler · 2019-05-31T17:34:06Z

bors r+

4153: CBR-525: Fix failure to sync issue when switching to OBFT r=disassembler a=erikd ## Description Fix issue that caused the node to loose sync with the chain in OBFT era and then fail to re-sync once out of sync. ## Linked issue https://iohk.myjetbrains.com/youtrack/issue/CBR-525 - [x] CHANGELOG entry has been added and is linked to the correct PR on GitHub. ## Testing checklist  - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. ## QA Steps Synced the OBFT testnet numerous times. Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com> Co-authored-by: Michael Hueschen <michaelhueschen@gmail.com> Co-authored-by: Samuel Leathers <samuel.leathers@iohk.io>

disassembler · 2019-05-31T19:19:03Z

bors r+

4153: CBR-525: Fix failure to sync issue when switching to OBFT r=disassembler a=erikd ## Description Fix issue that caused the node to loose sync with the chain in OBFT era and then fail to re-sync once out of sync. ## Linked issue https://iohk.myjetbrains.com/youtrack/issue/CBR-525 - [x] CHANGELOG entry has been added and is linked to the correct PR on GitHub. ## Testing checklist  - [x] I have added tests to cover my changes. - [x] All new and existing tests passed. ## QA Steps Synced the OBFT testnet numerous times. Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com> Co-authored-by: Michael Hueschen <michaelhueschen@gmail.com> Co-authored-by: Samuel Leathers <samuel.leathers@iohk.io>

iohk-bors · 2019-05-31T19:39:38Z

Build succeeded

erikd requested review from disassembler, mhuesch and intricate May 30, 2019 07:34

erikd force-pushed the erikd/obft-validation branch 5 times, most recently from 464bbdf to ff01d02 Compare May 30, 2019 08:46

mhuesch force-pushed the erikd/obft-validation branch from ff01d02 to 9a5a3db Compare May 30, 2019 17:16

intricate reviewed May 30, 2019

View reviewed changes

mhuesch approved these changes May 30, 2019

View reviewed changes

erikd commented May 30, 2019

View reviewed changes

chain/src/Pos/Chain/Block/Header.hs Show resolved Hide resolved

intricate reviewed May 30, 2019

View reviewed changes

db/src/Pos/DB/Block/Logic/VAR.hs Show resolved Hide resolved

intricate reviewed May 30, 2019

View reviewed changes

chain/src/Pos/Chain/Block/Header.hs Outdated Show resolved Hide resolved

erikd and others added 3 commits May 31, 2019 07:35

CBR-525: Implement splitByEpoch function

3ae21fe

With property tests.

CBR-525: Fix failure to sync issue when switching to OBFT

d1799ae

Co-authored-by: Luke Nadur <luke.nadur@iohk.io>

chain: Add a new version of LastBlkSlots data type

644b9cf

The old version of the struct was a list which made calculating the OBFT parameters highly inefficient. New version is much more efficient and has tests.

erikd force-pushed the erikd/obft-validation branch from 0488fd9 to 50e0eb5 Compare May 30, 2019 21:43

erikd and others added 3 commits May 31, 2019 07:57

Replace old LastBlkSLots data type with new one

4e93739

CBR-525: Document delegator/delegatee story

e1ed2fc

I also take the liberty of fixing a typo.

Update CHANGELOG

5abdfa0

erikd force-pushed the erikd/obft-validation branch from 50e0eb5 to 5abdfa0 Compare May 30, 2019 21:57

mhuesch and others added 2 commits May 30, 2019 22:57

CBR-525: Fix typos

2915ca7

stylish-haskell fixes

be04da9

intricate approved these changes May 31, 2019

View reviewed changes

iohk-bors bot merged commit be04da9 into develop May 31, 2019

iohk-bors bot deleted the erikd/obft-validation branch May 31, 2019 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBR-525: Fix failure to sync issue when switching to OBFT #4153

CBR-525: Fix failure to sync issue when switching to OBFT #4153

erikd commented May 30, 2019

mhuesch commented May 30, 2019 •

edited

Loading

intricate left a comment

mhuesch left a comment •

edited

Loading

mhuesch May 30, 2019

erikd May 30, 2019 •

edited

Loading

mhuesch commented May 30, 2019

disassembler commented May 31, 2019

disassembler commented May 31, 2019

iohk-bors bot commented May 31, 2019

CBR-525: Fix failure to sync issue when switching to OBFT #4153

CBR-525: Fix failure to sync issue when switching to OBFT #4153

Conversation

erikd commented May 30, 2019

Description

Linked issue

Testing checklist

QA Steps

mhuesch commented May 30, 2019 • edited Loading

intricate left a comment

Choose a reason for hiding this comment

mhuesch left a comment • edited Loading

Choose a reason for hiding this comment

mhuesch May 30, 2019

Choose a reason for hiding this comment

erikd May 30, 2019 • edited Loading

Choose a reason for hiding this comment

mhuesch commented May 30, 2019

disassembler commented May 31, 2019

disassembler commented May 31, 2019

iohk-bors bot commented May 31, 2019

Build succeeded

mhuesch commented May 30, 2019 •

edited

Loading

mhuesch left a comment •

edited

Loading

erikd May 30, 2019 •

edited

Loading