Node 11.1 integration#2043
Conversation
| let ProtVer m _ = SL.bprotver bhb | ||
| unless (m <= maxPV) $ | ||
| throwError (ObsoleteNodeCHAIN m maxPV) | ||
| unless (bhHSize <= fromIntegral @Word16 @Int (ccMaxBHSize ccd)) $ | ||
| throwError $ | ||
| HeaderSizeTooLargeCHAIN bhHSize (ccMaxBHSize ccd) | ||
| unless (bhBSize <= ccMaxBBSize ccd) $ | ||
| throwError $ | ||
| BlockSizeTooLargeCHAIN bhBSize (ccMaxBBSize ccd) |
There was a problem hiding this comment.
Is this what was there in SL.chainChecks before?
There was a problem hiding this comment.
right, I wanted to bring this up but got distracted - I have inlined chainChecks here because the upstream one in ledger doesn't work due to constraints.
there is a duplicate implementation also in Praos, and after chatting with @lehins we agree that it might be best to just downstream chainChecks here entirely.
Then PraosEnvelopeError could become EnvelopeError and replace the ChainPredicateFailure from upstream, since it's the same type duplicated. Would that make sense to you @jasagredo?
There was a problem hiding this comment.
Is this what was there in SL.chainChecks before?
In short chainChecks is not needed in ledger, it was always there only for Consensus consumption, which makes it more reasonable to move into Connsensus.
As @f-f pointed out, this implementation is exactly the same for Praos as it is for TPraos. I can give you a suggestion on how you could reduce this duplication, but I am sure you can figure out on your own. Now that both of those implementations are in Consensus it is obvious that they are the same and it should be trivial to deduplicate.
There was a problem hiding this comment.
I have unified the two checks and merged the two calls. The shared material is now in the new EnvelopeChecks module, have a look and let me know if you'd like a different approach!
geo2a
left a comment
geo2a
left a comment
There was a problem hiding this comment.
I have some superficial comments for now, I'll be back with a deeper review today or tomorrow.
Overall the direction looks good.
I've asked Claude to track down the Ledger PRs that tirggered these changes. Could you verify the list?
- #5560 —
EraBlockHeadertypeclasses deprecatingBHeaderView - #5632 — Forecast API (
TPraosLedgerView,forecastTo*LedgerView) - #5572 — Streaming initial-funds (
ShelleyExtraConfig,sgExtraConfig,SomeHasFSplumbing) - #5777 — Full streaming for stake pools / credentials / DReps
- #5764 — Queries moved from consensus to ledger (
queryStakePoolRelaysetc.) - #5716 —
applyTickextracted into its ownApplyTickclass - #5733 — Dijkstra block CDDL +
blockBodySizerename - #5719 — Protocol-version validation in
createInitialState
| let ProtVer m _ = SL.bprotver bhb | ||
| unless (m <= maxPV) $ | ||
| throwError (ObsoleteNodeCHAIN m maxPV) | ||
| unless (bhHSize <= fromIntegral @Word16 @Int (ccMaxBHSize ccd)) $ | ||
| throwError $ | ||
| HeaderSizeTooLargeCHAIN bhHSize (ccMaxBHSize ccd) | ||
| unless (bhBSize <= ccMaxBBSize ccd) $ | ||
| throwError $ | ||
| BlockSizeTooLargeCHAIN bhBSize (ccMaxBBSize ccd) |
There was a problem hiding this comment.
Is this what was there in SL.chainChecks before?
In short chainChecks is not needed in ledger, it was always there only for Consensus consumption, which makes it more reasonable to move into Connsensus.
As @f-f pointed out, this implementation is exactly the same for Praos as it is for TPraos. I can give you a suggestion on how you could reduce this duplication, but I am sure you can figure out on your own. Now that both of those implementations are in Consensus it is obvious that they are the same and it should be trivial to deduplicate.
f-f
force-pushed
the
f-f/prepare-11.1
branch
2 times, most recently
from
2ead6f4 to
52baebd
Compare
| -- TODO: cardano-base PR #635 exposed 'minSigPoPDST' and 'minVerKeyPoPDST' but not the | ||
| -- corresponding plain-signature ciphersuites. Should these be upstreamed as well? |
There was a problem hiding this comment.
@perturbing should minSigSignatureDST be upstreamed or how can we replace it here? I'd like to do something with it because right now we have to import Cardano.Crypto.DSIGN.BLS12381.Internal to make it work and I'd rather not
There was a problem hiding this comment.
You should switch to
import Cardano.Crypto.DSIGN.BLS12381
( BLS12381MinSigDSIGN
, minSigPoPDST
)But that is probably not yet on CHaP.
Also note that in this PR you switch from
{ blsSignContextDst = Just "BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_POP_"
to
{ blsSignContextDst = Just "BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_NUL_"
But this is wrong. You should use exactly
There was a problem hiding this comment.
Thanks for having a look!
Note that I did not switch to the plain one, I just removed the PoP variant from here because we are using the one in base.
I have now removed both and moved to using PoP everywhere. Could you have another look?
5 participants
Description
Integration for release 11.1 - here we update to the latest
base/network/ledgerThe patch should build but it's very much a starting point for discussion – I am not sure at all of all the changes and in fact I am sure that some of them are just not right.
So please help me out with filling the blanks and feel free to take over and push here directly to fix things up.