Leios prototype: EB validation in vote logic - #2235
Conversation
f62c00e to
66045e7
Compare
b9c5e42 to
221836f
Compare
nfrisby
left a comment
There was a problem hiding this comment.
First pass, which excluded two modules: ThreadNet tests and LeiosVoting itself. (That's as far as I got before my dev code finished compiling :D)
| -- | ||
| -- NOTE: the cache's tag does not record which protocol version the tx was | ||
| -- validated under, so an EB straddling a protocol-version change could skip | ||
| -- a static check whose rules changed. The mempool has the same exposure, and |
There was a problem hiding this comment.
The mempool has the same exposure, and the cache only retains a short window of EBs.
Neither of those justifies tolerating this. In particular, a bad tx in the Mempool doesn't lead to a bad tx on chain (RB validation will reject the block). But voting for a bad tx in an EB does.
My initial instinct is that we should change the CertRB validity rules to be invalid if they're attempting to certify an EB from the previous era. (In other words, the first block of an era must not be a CertRB.)
There was a problem hiding this comment.
Did we finally arrive to the dreaded topic of cross-era concerns x) Let's use this to dig a bit through this...
What does it mean to certify an EB that from the previous era? Which rules should be applied to that EB? Original era, or the new one?
My take is; We're never actually in the new era when certifying!
To cross an era we must tick right? But we verifyLeiosCert and applyLeiosClosure before ticking!
So in effect, we're always applying the rules to EBs and Certs of a single era (that they originated in). In other words, no such thing as cross-era EB/Cert.
Am I missing something?
There was a problem hiding this comment.
After discussing in Slack, we do tickThenApply that invokes BBODY Ledger rule AFTER TICK which means in the new ERA. I find that to be an implementation problem, and we should instead:
verifyLeiosCertand invoke a special LedgerBCertBodyapplyLeiosClosuretick
There was a problem hiding this comment.
I, of course, have only worked and reviewed the code .. not the comments that the 🧞 produced while we paired. I do agree with that we don't want to allow certification across era boundaries. I would even say we must not certify across epoch boundaries -> so protocol parameters can be assumed static too.
There was a problem hiding this comment.
@bladyjoker not taking the bait because I truly believe it's not worth it to worry about cross era / epoch certification at this point.
nfrisby
left a comment
There was a problem hiding this comment.
I'm Requesting Changes primarily for the commit message and comments that libel against BlockchainTime :P
I've also made some suggestions that seem quite preferable (eg only acquiring the LeiosTxCache log twice per EB instead of thousands of times, a few helpful explanatory code comments, etc)
| Added weight mCert -> do | ||
| traceWith tracer TraceLeiosVoted{vote, weight} | ||
| traceWith tracer TraceLeiosVoteAcquired{vote} | ||
| -- Trace certification whenever the tally crosses |
There was a problem hiding this comment.
- I'd be surprised if the log-writer deduped. So that seems like we'd rather have
addVotetrack exclude the cert/set a flag/etc when first emitting. - This comment says it fires "whenever the tally crosses ". Which can only happen once. But then it says it may fire again. So either it is already deduped or the "when it crosses" is the wrong wording (crossings only happen once, right? Unless concurrency is the only cause of dupes?).
There was a problem hiding this comment.
The mCert is only set when we first certify. I don't understand your comment / what are you wanting me to change?
221836f to
c0d0e62
Compare
bladyjoker
left a comment
There was a problem hiding this comment.
Heyo,
I'd say the most salient comment is about the the model of Voting thread.
One point I think holds pretty well is that it should be driven by "selection change" events, just like the Mempool (not on AcquiredEbTxs).
The thread then waits for the status of the associated EB announcement to be "closure acquired" and proceeds to vote. This part should be cancellable due to either deadline or selection change.
I think this even removes the need for timers maybe? Could be simplified?
Another missing part is that we probably don't have the ebAnnouncementStatusVar :: TVar (Map LeiosPoint AnnouncementStatus) just laying around. However, we do have all the events we need to make that happen in subscribeEbNotifications, a thread that monitors these notifications and maintains the ebAnnouncementStatusVar var sounds doable.
Wdyt?
| mconcat | ||
| [ "kind" .= Aeson.String "LeiosVoteScheduled" | ||
| , "ebHash" .= prettyEbHash ebHash | ||
| , "ebSlot" .= ebSlot |
There was a problem hiding this comment.
Why are we still carrying the slot for? This should be RbHash.
There was a problem hiding this comment.
Do you want me to drop the LeiosPoint / eb tracing or add the RbHash or do both?
| mconcat | ||
| [ "kind" .= Aeson.String "LeiosEbValidated" | ||
| , "ebHash" .= prettyEbHash ebHash | ||
| , "ebSlot" .= ebSlot |
There was a problem hiding this comment.
Same as before, this should be RbHash
There was a problem hiding this comment.
Remember in case of forks that have the same EB announced at the same slot this becomes ambiguous.
| { scheduleVoteTime = \point -> do | ||
| extLedger <- atomically $ ChainDB.getCurrentLedger chainDB | ||
| case slotOnset lcfg (ledgerState extLedger) (pointSlotNo point) of |
There was a problem hiding this comment.
This is bothering me, what's the relation between extLedger and point?
If votes are cast per announcement on our currently selected ledger state, then these two can differ here in a way that's suspicious.
For the sake of argument,
extLedgercontains thePraosStatethat has an associatedRbHash,SlotandEbAnnouncement.pointhas aSlotandEbHash
This method doesn't assure that current extLedger is carrying the EbAnnouncement for which we're voting.
In fact, if we keep it this way, if extLedger changed because our selection changed, we shouldn't proceed with voting right?
There was a problem hiding this comment.
This is not a LeiosPoint, but a Point. We do check the announcement when it's time to vote - selection could change in between! See the ChainTipDoesNotAnnounce case
| let keys = foldMap (getTransactionKeySets . snd) closure | ||
| values <- resolveValues keys | ||
| -- Determine which txs we can just reapply (the cache hits) | ||
| decided <- withLookupTx txCache $ \look -> mapM (decide look) closure |
There was a problem hiding this comment.
I didn't look how this works, but I'd be surprised if this does batched lookups? This is ideally a Set.intersection txCache closureTxHashes.
Currently, we're O(length closure * O(look)) which I assumes is at best O(length closure * O(log (length txCache))).
| RelativeTime -> | ||
| m (Either LeiosNotVotedReason ()) | ||
| goVote leiosConn sk point deadline = do | ||
| let vk = deriveVerKeyDSIGN sk |
There was a problem hiding this comment.
Bind vk once outside and pass it as argument
There was a problem hiding this comment.
No. That would then be a "bigger than necessary signature" and the two values could drift -> I would need to check whether the vk matches the sk
| forever $ | ||
| atomically ((Left <$> waitAcquiredEbTxs) <|> (Right <$> waitNextVoteTime)) | ||
| >>= \case | ||
| Left point -> scheduleVoteTime point |
There was a problem hiding this comment.
I'm not sure about this, why are we scheduling a vote for every acquired EB?
Let me take a stab at the logic of this Voting thread...
In my view, Voting thread should subscribe on new selections (just like the Mempool) and that should be the main driver.
It then proceeds to relate EB announcements and their status to the current selection
- Check if there's an EB announcement for the current praos/ledger state
2. If we continue using Praos headers for announcement then check the PraosStatepraosStateLeiosAnnouncement
3. If we use theLeiosNotifyfor EB announcements, then look it up there - If we found an EB announcement, check it's status
3. "waiting on EB body" -> " EB body acquired" -> "EB closure acquired" - Wait on the EB announcement status to reach "EB closure acquired"
- (go)Vote if we're not passed the deadline
This entire procedure must be interruptible to avoid wasting resources:
- Deadline is exceeded
- New selection comes about
There was a problem hiding this comment.
There is multiple options in how to structure the voting loop. I originally picked the form of being driven by EB announcements / their acquisition and only check whether it matches our selection at the time of voting (this is important as it can change while we wait for the voting period). I didn't change it in this PR (only refactored the semantics). Feel free to rewrite to the semantics you suggest and see if it is more clear. I certainly won't do it in this PR.
This entire procedure must be interruptible to avoid wasting resources:
I agree that this is the challenge it's not done well right now
| now <- lift $ systemTimeCurrent systemTime | ||
| when (now > deadline) $ | ||
| throwE TooLate |
There was a problem hiding this comment.
I'd rather see a thread cancelled due to a timeout then this check.
Why? Because this check could be added at any point in this function, actually, it makes sense to add it everywhere to stop ASAP. That just tells me that we should use timeout and cancel thread execution when the deadline is hit.
There was a problem hiding this comment.
Yeah the current logic is more synchronous than it needs to be. The same is true for the forge loop and other aspects of the system. Synchronous is often easier to write and read :)
c0d0e62 to
7b34aa8
Compare
79ebf9d to
fcfd9f0
Compare
e84548d to
a51c7b6
Compare
a51c7b6 to
9d45eff
Compare
dnadales
left a comment
There was a problem hiding this comment.
Full disclosure, these concerns were raised by Claude. I filtered them, and I'm posting here the ones that seemed to make sense.
| @@ -14,60 +15,99 @@ module LeiosVoting | |||
|
|
|||
There was a problem hiding this comment.
(This is really about the export list just above, LeiosVoting.hs:11-14, which isn't in the diff so I can't anchor there.)
Should we name what we want to export?
module LeiosVoting publishes everything, and the module is in exposed-modules (ouroboros-consensus.cabal:123), so this PR newly makes VoteTimers(..), newVoteTimers, slotOnset, and the (?>=) helper at line 489 part of the library's API. validateEbClosure and EbClosureVerdict(..) we do want, since Test.LeiosVoting imports them.
The wildcard predates this PR, so this isn't really something the PR introduced. But (?>=) is a very general name to be handing out from a module about voting, and it's new here. Would an explicit list be worth doing while we're in the file? Something like:
module LeiosVoting
( runLeiosVoting
, validateEbClosure
, EbClosureVerdict (..)
, HasLeiosVoting (..)
) whereThere was a problem hiding this comment.
I don't really like explicit export lists for applications (We are implementing the voting logic for a node here, not a generally reusable library)
Is this a Must, Should or Could comment?
Voting applied each tx with 'DoNotIntervene', which is the mempool's be-generous-to-remote-peers setting. For Alonzo and later that is not a courtesy, it rewrites the transaction: applyAlonzoBasedTx forces isValid true, and on a ValidationTagMismatch retries with it false. So a Plutus tx declaring isValid true whose scripts fail validated here as a collateral-only tx, and we voted for the EB carrying it. The apply path does none of that. applyLeiosClosure runs the LEDGER rule with ValidateNone, script evaluation sits inside when2Phase and is skipped, and the UTxO effect branches on the declared flag -- so the chain consumes the tx's whole input set. The transaction we validated and the transaction the chain applies were not the same one, which is precisely the trust applyLeiosClosure cites the certificate for. 'Intervene' leaves the flag alone and lets the mismatch escape as an error, so we now fail closed on exactly the txs the apply path would mishandle. Reported by @dnadales in review of #2235.
All from @dnadales's review of #2235. Fail a vote rather than the node. addVote returning anything but Added was an error call, which under forkLinkedThread takes the node down. This PR made it reachable: seatId is resolved from the forker, addVote re-reads the committee from the current selection, and closure validation now sits between the two, so an epoch boundary crossed in that gap silently makes the positional LeiosSeatId someone else's seat. It is now a VoteRejected reason, traced like the others. That is containment, not a fix, and the TODO beside it says so: signing first and finding out afterwards is the wrong shape. We should establish that we are still on the same committee, and still hold that seat, before casting the vote. Check the deadline before validating, not only after. scheduleVoteTime arms on acquisition and AcquiredEbTxs only fires when the last tx of a closure lands, so a closure completing late gives a negative delay and the timer fires already expired. We were spending ~1.5s applying a closure for a vote that was dead before the timer existed. The check after validation stays: it is the one that judges the window by the clock at signing time. Commit every notification read. Looping past AcquiredEb inside the STM transaction meant a run ending in retry discarded the read-end write, so skipped messages came back to be skipped again on the next wake. It now returns Maybe and the caller ignores Nothing. Also: say what scheduleVoteTime actually does with an already-open window now that the max 1 clamp is gone, and unescape a haddock pipe.
Both from @dnadales's review of #2235. The harness answered every table read with the whole initial UTxO, ignoring the keys it was asked for. Since applyMempoolDiffs restricts the diffs rather than the values, the key sets could have been empty and all four properties would still have passed -- confirmed by blinding them, which now fails three. Restricting the reply also gives the transitive-closure TODO in validateEbClosure something to fail against once we get to it. And prop_stopsAtFirstFailure claimed to cover a tx after the failure while the generator put the failing tx last, so abandon skipping the rest of the closure was never exercised. The generator now emits a tail that would have applied if reached, and the property asserts it stays untagged.
9d45eff to
63891a3
Compare
'resolveLeiosClosure' returns [(TxHash, GenTx blk)] instead of [GenTx blk]. 'leiosDbLookupEbClosure' already reads the hash out of the LeiosDb and the Dijkstra instance was throwing it away; the voting thread is about to need it to key the LeiosTxCache. Callers that only want the transactions drop it with 'map snd'. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebuilds the 'Validated' token for a closure tx, so a caller holding evidence that the tx was already validated can pick 'reapplyTx' (which skips only the static checks) over a full 'applyTx'. The production LeiosTxCache cannot hand back such a token itself: its table packs a refcount and a three-valued tag into a Word64, so 'lookupTx' yields 'Maybe (Either () ())' and nothing more. The tag is the evidence; the tx we already resolved is the payload. For Dijkstra that makes rebuilding it a tx-id hash, and 'SL.reapplyTx' derives the state-dependent annotation from the state at hand, so nothing stale rides along. No caller yet -- the voting thread picks it up two commits from here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sed tx
The voting thread signs a vote for every EB whose closure it acquires without
ever looking at the transactions (ouroboros-leios#1016), so a committee member
will happily certify an EB full of invalid txs. This is the test that says so.
'prop_leios_invalid_eb' runs 4 nodes with node 1 adversarial: it appends a tx
spending a phantom TxIn to 'fbEbTxs'. Only the EBs it announces are poisoned --
'fbRbTxs' is untouched, so its RBs stay valid, the chain keeps growing, and
honest nodes' own EBs still certify.
To let a test install a misbehaving node, 'runThreadNet'' takes a per-node tweak
of 'TestNodeInitialization' and 'runThreadNet' passes the identity. The tweak
ranges over the whole 'TestNodeInitialization' rather than just its
'BlockForging', so the next test that needs a misbehaving node can reach the
protocol info or the crucial txs without changing this plumbing again.
The bogus tx fails on a state-dependent check (BadInputsUTxO), not a missing
witness: a witness is a static check, which is exactly what the reapply path is
allowed to skip, so a witness-only failure would not pin down the behaviour we
care about.
'EbTxsInvalid' and 'TraceLeiosEbValidated' are the observable contract the test
asserts on. Nothing emits either yet, hence:
invalid endorsed tx is not certified: FAIL
[failed] poisonedNeverCertified
a poisoned EB was certified
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes ouroboros-leios#1016. 'validateEbClosure' folds an EB's closure onto the
announcing RB's unticked ledger state -- ticked to the EB's slot, which mirrors
what chain-sel does when a later RB certifies it -- through the ordinary Mempool
API. A tx the LeiosTxCache has tagged validated goes through 'reapplyTx', which
re-runs every state-dependent check but skips the static ones; anything else
goes through a full 'applyTx'. On the first failure the EB gets no vote.
Two things the shape of this is trying to protect:
* It runs strictly after the three cheap gates (TooLate,
ChainTipDoesNotAnnounce, NotOnCommittee), so an EB we would not vote for is
never validated.
* The cache lookups happen in one short pass under the cache's lock and the
ledger work runs outside it, so validating a 15k-tx closure cannot stall
LeiosFetch's inserts behind us.
Txs validated here are tagged applied afterwards, so a later EB sharing them
reapplies instead of validating from scratch. In the honest ThreadNet run 18%
of closures are fully reapplied (a forger's own EB) and 14% fully applied, the
rest partial; the remaining full validation is what a mempool-to-cache sync
(the other half of ouroboros-leios#1021) would take away.
'TraceLeiosEbValidated' reports the split, because the issue asks us to watch
what this costs certification. Worth watching on the devnet: validation is
inline in the voting loop, so it serialises EB voting and eats into L_vote.
invalid endorsed tx is not certified: OK
poisoned EB turned down on: 100% EbTxsInvalid
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
In an honest net every acquired closure must apply against the announcing RB's ledger state, so 'propClosuresValidate' fails if any voter reports 'EbTxsInvalid'. A rejection there does not mean a tx was bad -- it means the voting thread validates against the wrong state or the wrong slot. Also tabulates the reapply-vs-apply split, so the tx-cache's hit rate shows up in the run output rather than having to be dug out of the traces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ThreadNet property cannot see any of this: it observes whether an EB gets certified, not what the tx-cache ends up holding, so it passes whether or not we record what we validated. These tests look at the cache directly. To get the function under test, the forker plumbing moves out to the caller: 'validateEbClosure' now takes the announcing RB's ledger state and a 'resolveValues' callback -- the same shape 'resolveAndApplyLeiosClosure' already uses on the apply path -- instead of a ChainDB. 'EbClosureNoLedger' goes with it, since the caller now handles a missing forker directly. Mock blocks are the vehicle: what matters here is the orchestration -- which ledger rule each tx goes through, and what is recorded -- not the ledger rules, and the mock UTxO ledger already has generators for valid and invalid txs. That needs two real 'ResolveLeiosBlock' methods on 'SimpleBlock', narrowed to @ext' ~ ext@ because that is where 'GenTx' is defined; the certificate-shaped methods keep their defaults. Four properties: * a fresh closure is validated in full, and every tx ends up tagged * a second pass over the same closure reapplies instead of applying, which is the cache actually being consulted * a closure that fails part-way still records everything validated before the failure * the failing tx itself is not recorded Checked they bite, by mutation. Dropping the write-back entirely fails three of the four (the last passes vacuously -- it is only meaningful next to the others). Restoring the specific bug this fixed -- collect, and record only if the whole closure applies -- fails exactly the third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems with the old gates, both flagged by the FIXME this removes. They counted slots, but the protocol parameters they stub for are diff times, not slot counts -- there is no whole-slot value to count against. Both gates are now wall-clock offsets from the announcing slot's onset, which 'slotOnset' reads from the ledger's hard fork summary: 'lHdrWait' before a vote may be cast, 'lVoteWindow' after that before it is too late. Same 3 and 4, now seconds rather than slots. And the deadline was checked before the closure was validated. Validating a 15k-tx closure takes real time, so that reading was stale by the time we were ready to sign. 'decideVote' now reads the clock after validation. The cheap checks still come first, so an EB we would not vote for is still never validated. Consequences for the loop. The L_hdr gate can no longer be an STM retry on 'getCurrentSlot', so it waits on a 'registerDelay' timeout raced against 'takeAcquisition' instead. That keeps the loop reactive: an EB turning up late with an older slot does not have to sit out the wait of the one we happened to be watching, which a plain 'threadDelay' would have cost us. 'ingest' drains arrivals without blocking when there is already something pending, and every pass re-reads the earliest, so the decision is always taken against the current head of the queue. 'slotOnset' returns 'Either PastHorizonException', not a 'runQueryPure' that throws. The loop is the wrong place to discover that a slot is past the horizon -- it cannot happen for an EB announced on our own chain, whose slot is at or behind the tip we just read the summary from, but "cannot happen" inside a 'forever' is exactly what should not be an exception. The point is dropped and traced rather than retried, so a wedged conversion cannot spin the thread. 'runLeiosVoting' takes the 'SystemTime' in place of the 'BlockchainTime' it no longer needs, and gains the 'MonadTimer' constraint 'initNodeKernel' already carried. Six unit tests for 'decideVote', which is now separable from the loop's state queries. Mutation-checked: moving the deadline check back before validation fails exactly "reads the clock after validating". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'runLeiosVoting' had grown three layers doing one job. 'validateClosure' was plumbing around 'validateEbClosure', 'decideVote' decided, and 'voteOn' called the two and cast the vote, threading the validation counts through 'decideVote' purely so 'voteOn' could trace them. That is one function's worth of work, so it is one function: 'goVote' reads the committee and ledger state, validates, checks the deadline and casts, with every way of not voting leaving via 'throwE' and the reason traced once at the call site. 'goVote' lives in the 'where' clause, taking the three things it cannot reach from there -- the LeiosDb connection and the derived voting key and signer. It touches neither the pending set nor the notification channel, so the branch-local 'let' is now only what genuinely closes over those: 'startVoting' with the queue plumbing and the scheduling loop. One forker read where there were two reads and a race. The header state that has to announce the EB and the ledger state its closure has to apply to now both come from a single forker opened at 'VolatileTip', rather than reading the ChainDB's ledger and then opening a forker at its tip -- which could move in between. 'withForkerAt' keeps that in plain 'm'; the early-exit flavoured 'withReadOnlyForkerAtPoint' would drag 'WithEarlyExit' through the loop, and being 'MaybeT' underneath it cannot carry an abstention reason back out anyway, which forced the reason to be traced twice. 'runLeiosVoting' takes the 'LedgerConfig' rather than the whole 'TopLevelConfig' -- both uses were 'configLedger', and the signing key already arrived separately. Removes the six 'decideVote' unit tests: the function no longer exists as a seam, and 'goVote' closes over too much to reach from a test. What is no longer pinned is the ordering -- that validation runs before the clock is read, and that a failed cheap check never validates. The four 'validateEbClosure' tests are untouched, and the ThreadNet adversary property still covers rejection end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is only relative time to avoid a dependency on SystemStart in the vote loop.
The vote gates are durations: the protocol parameters they stub for are diff times, not slot counts, so they are measured from the announcing slot's onset rather than counted against the current slot. The haddock on 'slotOnset' now says that and nothing more -- it previously offered a rationale about the current slot that did not hold. Also from review: 'committing' reworded to 'issuing our vote'; TODOs on both constants about coming from the ledger state rather than staying constants; and a note on why the timer delay is clamped to 1us, which is what an already-open window produces. In the tests, the claim that an entry can only be upgraded after being marked acquired is false -- a tx taken straight from the mempool reaches Applied without that step -- so the comment now says what the harness does rather than asserting an invariant. The 'NoThunks' remark is moved onto the 'force' it explains, where it is no longer a non-sequitur between two sentences about the adversary. The tabulated reapply/apply split no longer claims a forger's own EB should be entirely reapplied: that is not guaranteed by the implementation and nothing depends on it. Finally, the closure-validation property now says why a rejection implicates the voter rather than the transactions: 'partitionMempool' cuts both the RB's and the EB's transactions from a single mempool snapshot, which is internally consistent, and the announcing RB is that snapshot's state plus its own share of the split -- so the EB's share applies against the RB's post-state by construction.
… use site Review points, in three parts. The harness marked every closure tx acquired before validating, so the voting logic was never asked what to do when the cache holds nothing for a tx. That is reachable -- a tx can arrive straight from the mempool and go to Applied without passing through the acquired step, and a just-restarted node may hold neither -- and a miss must behave exactly like an unapplied entry. Each property now draws a per-tx mask deciding what the cache has seen, and the first one tabulates it: at 400 tests the split runs from 0/4 to 4/4, with all four passing throughout. 'invalidEbTx' is now an ordinary invalid transaction, with 'assumeValidatedClosureTx' applied where the adversary uses it. The lie -- that the tx is mempool-validated, so the forge will endorse it -- is told to the forge logic and nowhere else, which is the point of the fixture. The TODO about starting validation earlier conflated two changes. Split: validate once the whole closure has arrived rather than when the vote window opens, and validate as the closure streams in. The first is much the cheaper and the second subsumes it; neither is attempted here. Also unbreaks consensus-test, which had not compiled since 'recordAnnouncedEb' gained its onset argument. The fetch invariants do not read the onset, so they pass SNothing.
…r tx 'recordValidated' took the LeiosTxCache lock for every transaction it validated, so a full closure acquired it ~13.5k times. The hashes are now accumulated as the fold runs and written in a single 'withLockedInsertAppliedTx'. The reason it was per-tx still holds and is preserved: a closure that fails part-way must keep the tags for everything validated before the failure, since that work was done against a real ledger state and holds regardless of the transaction that sank the EB. So the flush happens on both exits -- the valid one and 'abandon' -- rather than only at the end. Removing it from 'abandon' fails "records the txs validated before a failure" on the first test case, which is the guard for exactly this. Order within the batch is irrelevant, each entry being an independent tag, so the accumulator is not reversed.
Voting applied each tx with 'DoNotIntervene', which is the mempool's be-generous-to-remote-peers setting. For Alonzo and later that is not a courtesy, it rewrites the transaction: applyAlonzoBasedTx forces isValid true, and on a ValidationTagMismatch retries with it false. So a Plutus tx declaring isValid true whose scripts fail validated here as a collateral-only tx, and we voted for the EB carrying it. The apply path does none of that. applyLeiosClosure runs the LEDGER rule with ValidateNone, script evaluation sits inside when2Phase and is skipped, and the UTxO effect branches on the declared flag -- so the chain consumes the tx's whole input set. The transaction we validated and the transaction the chain applies were not the same one, which is precisely the trust applyLeiosClosure cites the certificate for. 'Intervene' leaves the flag alone and lets the mismatch escape as an error, so we now fail closed on exactly the txs the apply path would mishandle. Reported by @dnadales in review of #2235.
All from @dnadales's review of #2235. Fail a vote rather than the node. addVote returning anything but Added was an error call, which under forkLinkedThread takes the node down. This PR made it reachable: seatId is resolved from the forker, addVote re-reads the committee from the current selection, and closure validation now sits between the two, so an epoch boundary crossed in that gap silently makes the positional LeiosSeatId someone else's seat. It is now a VoteRejected reason, traced like the others. That is containment, not a fix, and the TODO beside it says so: signing first and finding out afterwards is the wrong shape. We should establish that we are still on the same committee, and still hold that seat, before casting the vote. Check the deadline before validating, not only after. scheduleVoteTime arms on acquisition and AcquiredEbTxs only fires when the last tx of a closure lands, so a closure completing late gives a negative delay and the timer fires already expired. We were spending ~1.5s applying a closure for a vote that was dead before the timer existed. The check after validation stays: it is the one that judges the window by the clock at signing time. Commit every notification read. Looping past AcquiredEb inside the STM transaction meant a run ending in retry discarded the read-end write, so skipped messages came back to be skipped again on the next wake. It now returns Maybe and the caller ignores Nothing. Also: say what scheduleVoteTime actually does with an already-open window now that the max 1 clamp is gone, and unescape a haddock pipe.
Both from @dnadales's review of #2235. The harness answered every table read with the whole initial UTxO, ignoring the keys it was asked for. Since applyMempoolDiffs restricts the diffs rather than the values, the key sets could have been empty and all four properties would still have passed -- confirmed by blinding them, which now fails three. Restricting the reply also gives the transitive-closure TODO in validateEbClosure something to fail against once we get to it. And prop_stopsAtFirstFailure claimed to cover a tx after the failure while the generator put the failing tx last, so abandon skipping the rest of the closure was never exercised. The generator now emits a tail that would have applied if reached, and the property asserts it stays untagged.
63891a3 to
9d81c7f
Compare
…#2235, #2245) Merges the stacked series, bottom to top: * #2188 Leios: add LeiosTxCache, but don't rely on it at all yet * #2237 Rewrite the LeiosFetch decision logic * #2235 Leios prototype: EB validation in vote logic * #2245 LeiosDb: bound the WAL, remove the write hotspot, stop dying on lock contention IMPORTANT: the schema change in #2245 has no migration. An existing database is not readable by this version and requires a wipe & resync.
Resolves and validates EB closures in
runLeiosVoting.This is using the
TxCachefrom #2188 to determine whether we need toapplyTxorreapplyTxvia the existingLedgerSupportsMempoolAPI.As this change is heavily restructuring the Leios voting loop, I realized that we incorrectly used the
currentSlotto determine whether voting timed out -> this is wrong! The current slot (derived viaChainDB.getCurrentLedger) is only ever advancing when the tip advances. Thus, the "time" was lagging quite a lot. Changed the semantics determine onset and deadline of voting period using the wall clock (viaslotToWallclock).