Integrate ouroboros-network and cardano-ledger
#1314
Draft
+1,455
−564
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
15 tasks
neilmayhew
force-pushed
the
neilmayhew/release-srp
branch
5 times, most recently
from
9efce6b to
c27d199
Compare
neilmayhew
force-pushed
the
neilmayhew/release-srp
branch
3 times, most recently
from
36b2b58 to
e6ab67b
Compare
|
FYI, the cardano-ledger commit in the SRP is gone. I suspect the new one is IntersectMBO/cardano-ledger@3fe0221 or IntersectMBO/cardano-ledger@12f6aa6 (the merge commit of the PR). |
neilmayhew
force-pushed
the
neilmayhew/release-srp
branch
2 times, most recently
from
7cbd3f8 to
ef51726
Compare
lehins
reviewed
Dec 17, 2024
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Query/Types.hs
Show resolved
Hide resolved
ouroboros-consensus-cardano/src/unstable-shelley-testlib/Test/ThreadNet/TxGen/Shelley.hs
Outdated
Show resolved
Hide resolved
| let (ys, zs) = splitAt i xs | ||
| in ys ++ dropElemsAt (drop 1 zs) is | ||
| dropElemsAt xs is' = | ||
| let is = Set.fromList is' |
There was a problem hiding this comment.
IntSet is more efficient at handling Set of Ints
Suggested change
| let is = Set.fromList is' | |
| let is = IntSet.fromList is' |
Comment on lines
+72
to
+76
| , gcfBlockFetchGracePeriod :: Maybe Integer | ||
| , gcfBucketCapacity :: Maybe Integer | ||
| , gcfBucketRate :: Maybe Integer | ||
| , gcfCSJJumpSize :: Maybe Integer | ||
| , gcfGDDRateLimit :: Maybe DiffTime |
There was a problem hiding this comment.
Configuration should always try and use the types that will be used in the end. This way we can protect against invalid values when parsing the config instead of allowing for dangerous values that can lead to overflows. For example, by using SlotNo, we here we can protect against a very large value being placed in the config that will end up overflowing
Suggested change
| , gcfBlockFetchGracePeriod :: Maybe Integer | |
| , gcfBucketCapacity :: Maybe Integer | |
| , gcfBucketRate :: Maybe Integer | |
| , gcfCSJJumpSize :: Maybe Integer | |
| , gcfGDDRateLimit :: Maybe DiffTime | |
| , gcfBlockFetchGracePeriod :: Maybe DiffTime | |
| , gcfBucketCapacity :: Maybe Integer | |
| , gcfBucketRate :: Maybe Integer | |
| , gcfCSJJumpSize :: Maybe SlotNo | |
| , gcfGDDRateLimit :: Maybe DiffTime |
Comment on lines
+156
to
+159
| gbfcGracePeriod = fromInteger $ fromMaybe defaultBlockFetchGracePeriod gcfBlockFetchGracePeriod | ||
| csbcCapacity = fromInteger $ fromMaybe defaultCapacity gcfBucketCapacity | ||
| csbcRate = fromInteger $ fromMaybe defaultRate gcfBucketRate | ||
| csjcJumpSize = fromInteger $ fromMaybe defaultCSJJumpSize gcfCSJJumpSize |
There was a problem hiding this comment.
@facundominguez I have a couple of suggestions and some minor concerns about this:
csbcCapacityis already anInteger, sofromIntegeris redundant- using
maybeinstead offromMaybeallows for default values to have the expected type. - All conversions with
to/fromIntegerandfromIntegralshould have type annotations, because that way it is easier to spot dangers of overflow SlotNois backed byWord64, so there is danger of overflow here, so it would be better to use that type instead ingcfCSJJumpSize, as I suggested above.
Suggested change
| gbfcGracePeriod = fromInteger $ fromMaybe defaultBlockFetchGracePeriod gcfBlockFetchGracePeriod | |
| csbcCapacity = fromInteger $ fromMaybe defaultCapacity gcfBucketCapacity | |
| csbcRate = fromInteger $ fromMaybe defaultRate gcfBucketRate | |
| csjcJumpSize = fromInteger $ fromMaybe defaultCSJJumpSize gcfCSJJumpSize | |
| gbfcGracePeriod = maybe defaultBlockFetchGracePeriod (fromInteger @DiffTime) gcfBlockFetchGracePeriod | |
| csbcCapacity = fromMaybe defaultCapacity gcfBucketCapacity | |
| csbcRate = maybe defaultRate (fromInteger @Rational) gcfBucketRate | |
| csjcJumpSize = maybe defaultCSJJumpSize (fromInteger @SlotNo) gcfCSJJumpSize |
Comment on lines
+251
to
+252
| let is = Set.fromList is' | ||
| in map fst $ filter (\(_, i) -> not $ i `Set.member` is) (zip xs [0..]) |
There was a problem hiding this comment.
@nbacquey List comprehensions seem to be more readable, more compact and won't rely on list fusion to be efficient.
Also switching to IntSet will be more efficient and there are special functions in containers notMember x s, that can be used instead of not $ member x s
Suggested change
| let is = Set.fromList is' | |
| in map fst $ filter (\(_, i) -> not $ i `Set.member` is) (zip xs [0..]) | |
| let is = IntSet.fromList is' | |
| in [x | (x, i) <- zip xs [0..], i `IntSet.notMember` is] |
neilmayhew
force-pushed
the
neilmayhew/release-srp
branch
from
ef51726 to
9d27b2e
Compare
…kFetchConsensusInterface
* Addition of ChainSyncClientHandleCollection, grace period, and starvation event in BlockFetch * Plug `rotateDynamo` into `BlockFetchConsensusInterface` * Removal of `bfcMaxConcurrencyBulkSync` * Changes in blockfetch decision tracing
Port of IntersectMBO/ouroboros-network#2721 Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>
* Move Genesis-specific BlockFetch config to GenesisConfig * Introduce GenesisConfigFlags for interaction with config files/CLI * Add missing instances for Genesis configuration
* Mention that the objector also gets demoted * Edit note on Interactions with the BlockFetch logic * Expand the comments motivating DynamoInitState and ObjectorInitState Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>
* Run more repetitions of LoE, LoP, CSJ, and gdd tests * Print timestamps for node restarts * Disable boring timeouts in the node restart test * Wait sufficiently long at the end of tests * Expect CandidateTooSparse in gdd tests * Add a notice about untracked delays in the node restart test * Set the GDD rate limit to 0 in the peer simulator * Have the peer simulator use the default grace period for chainsel starvations * Relax expectations of test blockFetch in the BulkSync case * Allow to run the decision logic once after the last tick in the blockfetch leashing attack * Shift point schedule times before giving the schedules to tests * Accomodate for separate decision loop intervals for fetch modes * Accomodate for timer added in blockFetchLogic * Switch peer simulator to `FetchModeBulkSync` * Allow parameterizing whether chainsel starvation is handled * Add some wiggle room for duplicate headers in CSJ tests * Disable chainsel starvation in CSJ test
Co-authored-by: Nicolas Bacquey <nicolas.bacquey@tweag.io>
neilmayhew
force-pushed
the
neilmayhew/release-srp
branch
from
9d27b2e to
038f9d6
Compare
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.
Description
Integrate
ouroboros-networkandcardano-ledgerin preparation forcardano-noderelease10.2.0See IntersectMBO/cardano-node#6040 for overall progress