-
Notifications
You must be signed in to change notification settings - Fork 167
Implement block-submitting Imp functions #5404
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.
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
base: master
Are you sure you want to change the base?
Conversation
4961345 to
fa2b2b1
Compare
lehins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do like the direction where this is going, however this PR does need some changes.
I'll be happy to talk more about all the suggestions that I made in this PR
| ) => | ||
| NoThunks (Block h era) | ||
|
|
||
| instance (NFData h, NFData (BlockBody era)) => NFData (Block h era) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition should get a mention in the changelog
| } | ||
| deriving (Generic) | ||
|
|
||
| instance NFData BHeaderView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Adding chnagelog entry for NFData and Generic will be nice
| , impGlobals :: !Globals | ||
| , impEvents :: [SomeSTSEvent era] | ||
| , impRecordTxs :: Bool | ||
| , impEvents :: (Seq.Seq (Tx TopTx era), Seq.Seq (SomeSTSEvent era)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a big fan of sticking two totally unrelated concepts together into the same field named Events. In other words sequence of transactions has nothing to do with STS events, which is what this field was designed for.
FTR. we don't need to rely on the MonadWriter for this, in fact I'd rather avoid using it for recording transactions period. Writer was used for events, because that is also how it behaves in STS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, separating it seems much better, and not having to use Writer is very freeing
| itePostEpochBoundaryHookL = lens itePostEpochBoundaryHook (\x y -> x {itePostEpochBoundaryHook = y}) | ||
|
|
||
| instance MonadWriter [SomeSTSEvent era] (ImpTestM era) where | ||
| instance MonadWriter (Seq.Seq (Tx TopTx era), Seq.Seq (SomeSTSEvent era)) (ImpTestM era) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| instance MonadWriter (Seq.Seq (Tx TopTx era), Seq.Seq (SomeSTSEvent era)) (ImpTestM era) where | |
| instance MonadWriter (Seq.Seq (SomeSTSEvent era)) (ImpTestM era) where |
| Right (newState, events) -> do | ||
| impNESL . nesEsL . esLStateL .= newState | ||
|
|
||
| txsToRecord <- bool mempty (Seq.singleton txFixed) <$> use impRecordTxsL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please never use bool in the Ledger codebase. I understand the C developer in you is looking for ways to use ? like function 😁, but that function is seldom used in Haskell and tends to introduce bugs into the codebase, because not everyone remembers the order of which one is the True and which one is the False case.
This will be much more clear (combined with my other suggestions)
| txsToRecord <- bool mempty (Seq.singleton txFixed) <$> use impRecordTxsL | |
| modify' $ \impState -> | |
| case impRecordedTransactions impState of | |
| SNothing -> impState | |
| SJust txs -> impState {impRecordedTransactions = tx SSeq.:<| txs} |
| -- Return the block that was created using the transactions and any predicate | ||
| -- failures that are produced. | ||
| -- The transactions are not modified to make them balance. | ||
| trySubmitBlock :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in order to have it consistent with trySubmitTx, this function should accept a list of transactions that have not been fixed up and this function itself will call the simulateThenRestore in order to do all the fixing up and then submission as a block.
If we don't do that then usability of these functions will be limited.
That being said, naturally we'll need a help like this that will be capable of submitting transactions that have already been fixed up and ready to go into a block, I just think it needs to have a special name that is distinct from trySubmitBlock. Maybe trySubmitFixedUpBlock 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense.
There are two Conway tests that I've updated to use these functions (in eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp/BbodySpec.hs). The first doesn't require running the simulation itself, but the second definitely does, because the transactions all depend on each other. Also, the tests I'm porting from AlonzoBBODY work that way too. I think we'll find that this style will be more common, but time will tell.
Also, I found that it was just more convenient to write test code as if the transactions are being submitted, so that they use the same patterns as the rest of our test code, and then harvest them afterwards (by wrapping the transaction-generating code inside withTxsInBlock) rather than having to save the transactions explicitly. The fact that they're submitted in a block is then abstracted away from the code that creates the transactions.
I'll make all the other improvements (such as splitting events and transactions, and using SMaybe) first and then see how trySubmitBlock looks. Also, I'll push my latest changes to my AlonzoBBODY PR so that we can see what that test code looks like.
| submitFailingBlockM txs mkExpectedFailures | ||
|
|
||
| -- | Submit a list of transactions as a block that's expected to succeed. | ||
| -- The transactions are not modified to make them balance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function et al. should modify the sequence of transactions.
| -- The transactions are not modified to make them balance. |
| -- so that they're independent of each other | ||
| txIn <- freshKeyAddr_ >>= \addr -> sendCoinTo addr (Coin 100_000_000) | ||
| refIns <- replicateM n $ produceRefScript (fromPlutusScript plutusScript) | ||
| pure (txIn, NE.fromList refIns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my suggestion below
| pure (txIn, NE.fromList refIns) | |
| pure $ mkTxWithRefInputs txIn (NE.fromList refIns) |
| mkTxWithNScripts n | ||
| >>= fixupFees | ||
| >>= updateAddrTxWits | ||
| allInputs <- for txScriptCounts $ \n -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| allInputs <- for txScriptCounts $ \n -> do | |
| txs <- for txScriptCounts $ \n -> do |
| let | ||
| -- These txs will be grouped into a block | ||
| buildTxs = for_ allInputs $ uncurry submitTxWithRefInputs | ||
|
|
||
| withTxsInFailingBlock | ||
| buildTxs | ||
| [ injectFailure | ||
| ( BodyRefScriptsSizeTooBig $ | ||
| Mismatch | ||
| { mismatchSupplied = scriptSize * sum txScriptCounts | ||
| , mismatchExpected = maxRefScriptSizePerBlock | ||
| } | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my suggestion of making submitBlock et al. do the fixup, this becomes:
| let | |
| -- These txs will be grouped into a block | |
| buildTxs = for_ allInputs $ uncurry submitTxWithRefInputs | |
| withTxsInFailingBlock | |
| buildTxs | |
| [ injectFailure | |
| ( BodyRefScriptsSizeTooBig $ | |
| Mismatch | |
| { mismatchSupplied = scriptSize * sum txScriptCounts | |
| , mismatchExpected = maxRefScriptSizePerBlock | |
| } | |
| ) | |
| ] | |
| submitFailingBlock | |
| txs | |
| [ injectFailure | |
| ( BodyRefScriptsSizeTooBig $ | |
| Mismatch | |
| { mismatchSupplied = scriptSize * sum txScriptCounts | |
| , mismatchExpected = maxRefScriptSizePerBlock | |
| } | |
| ) | |
| ] |
Also abstract the interface for obtaining events from ImpTestM actions
e55ee00 to
2732862
Compare
2732862 to
5746746
Compare
Description
Add functions to:
Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).