Add optional expiry slot to Mock transactions #1564

nfrisby · 2020-02-03T21:02:26Z

Follow-up: prop_general is accumulating several block/protocol specific arguments; they probably deserve their own record type/class soon.

mrBliss · 2020-02-04T09:29:46Z

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Mock/UTxO.hs

+  = DoNotExpire
+  | ExpireAtOnsetOf !SlotNo
+  deriving stock    (Show, Eq, Ord, Generic)
+  deriving anyclass (Serialise, NFData, NoUnexpectedThunks)


Why is NFData (and that orphan) needed?

Tx derives it, and Expiry occurs in Tx. I didn't question why Tx derives it. (For the Tx pattern synonym, I suppose. Which uses force in order to have actual strictness of the [TxOut] argument, I guess?)

ouroboros-consensus/test-consensus/Test/ThreadNet/General.hs

Fixes #1565 and #1301. * Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This function will be much easier to test. We provide an implementation of `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the implementations by relying on the background thread for synchronising the Mempool with an updated ledger state. * Cache the `TickedLedgerState` after applying all transactions in the Mempool to it (#1301). Previously, we had to reapply all transactions whenever we added a new transaction. The new approach is much faster. * As we're keeping the `TickedLedgerState` in memory, make sure it is thunk-free. * Correct revalidation in (#1564) after explicitly removing transactions from the Mempool, see `revalidateTxsFor`. * Cleanup of conversion functions in `TxSeq`. * O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.

1599: Mempool: better revalidation and caching of the ledger state r=mrBliss a=mrBliss Fixes #1565 and #1301. * Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This function will be much easier to test. We provide an implementation of `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the implementations by relying on the background thread for synchronising the Mempool with an updated ledger state. * Cache the `TickedLedgerState` after applying all transactions in the Mempool to it (#1301). Previously, we had to reapply all transactions whenever we added a new transaction. The new approach is much faster. * As we're keeping the `TickedLedgerState` in memory, make sure it is thunk-free. * Correct revalidation in (#1564) after explicitly removing transactions from the Mempool, see `revalidateTxsFor`. * Cleanup of conversion functions in `TxSeq`. * O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure. Also includes a rewrite of the Mempool tests using the `Mock.Utxo`. Using the `Mock.Utxo` instead of the overly simplistic `TestTx` we were using allows for more complicated dependencies between transactions. This better resembles the real transactions, at the cost of some complexity. With the former transaction type, we weren't even able to trigger #1565. Co-authored-by: Thomas Winant <[email protected]>

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/TxGen/Mock.hs

mrBliss

LGTM

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/BFT.hs

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/TxGen/Mock.hs

mrBliss · 2020-03-10T08:45:25Z

ouroboros-consensus/ouroboros-consensus-mock/src/Ouroboros/Consensus/Mock/Ledger/Block.hs

-  reapplyTx = const updateSimpleUTxO
+  applyTx           = reapplyTx
+  reapplyTx _ tx tl =
+      case hasExpired of


I see CI is failing with a test related to tx/block validation. I believe you also have to take transaction expiration into account when validating blocks (applyLedgerBlock). You can move the expiration logic to some common place.

It shouldn't live in applyTx or applyLedgerBlock, but rather in the ledger rules.

See Lemma 3 in #1765 🙂

I pushed it into the ledger rules. I introduced the Ticked type used in a few modules; should it be renamed and/or located somewhere more general? I think of it (as commented) as a generalization of TickedLedgerState. @edsko @mrBliss

nfrisby · 2020-03-10T16:45:44Z

The latest CI failure was due to the unrelated #1779.

nfrisby · 2020-03-10T19:18:47Z

I think this is ready for merge, unless you'd like to see changes to the following.

the way I pushed the logic further into the ledger rule code
the Ticked type I used to do so

If not, I think this is ready for bors.

edsko

I must admit I am not a fan of the Ticked data type. Ticking a ledger state is something quite specific: apply ledger rules that advance the ledger state by time (update delegation state, swap out stake distribution, etc.). I don't know what a "ticket transaction" is, and I also don't really understand why we need a new concept. Perhaps we should discuss.

nfrisby · 2020-03-11T15:43:11Z

@edsko and I had a call. He pointed out a slightly higher-level location in which to check for expiry. Moving it there let me remove Ticked.

In order to relocate the check, I also simplified HasUtxo to HasTxs -- I need to access the actual Txs in order to check them for expiry. Is this too much of a concretization? I suppose I could add something about expiry to HasUtxo, but this simplification seems fine in the mock ledger code.

nfrisby · 2020-03-12T17:15:44Z

I just now rebased onto origin/master.

nfrisby · 2020-03-12T18:29:28Z

I just now rebased onto origin/bors/staging, to address the conflicts from the new PropGeneralArgs.

nfrisby · 2020-03-13T13:51:49Z

I rebased on origin/master.

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/BFT.hs

ouroboros-consensus/ouroboros-consensus-mock/src/Ouroboros/Consensus/Mock/Ledger/UTxO.hs

ouroboros-consensus/ouroboros-consensus-mock/src/Ouroboros/Consensus/Mock/Ledger/State.hs

nfrisby · 2020-03-16T14:47:09Z

Update: I addressed the comments and rebased onto master.

nfrisby · 2020-03-16T14:53:37Z

bors r+

iohk-bors · 2020-03-16T16:02:39Z

Build succeeded

nfrisby requested review from mrBliss and edsko February 3, 2020 21:02

nfrisby added mempool testing consensus issues related to ouroboros-consensus labels Feb 4, 2020

mrBliss reviewed Feb 4, 2020

View reviewed changes

mrBliss mentioned this pull request Feb 10, 2020

Mempool: better revalidation and caching of the ledger state #1599

Merged

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch 3 times, most recently from ed7217f to 8c20fd4 Compare March 9, 2020 17:24

nfrisby commented Mar 9, 2020

View reviewed changes

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/TxGen/Mock.hs Outdated Show resolved Hide resolved

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from 8c20fd4 to 99b2c2b Compare March 9, 2020 17:39

nfrisby requested a review from mrBliss March 9, 2020 17:39

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from 99b2c2b to e06dc41 Compare March 9, 2020 19:05

edsko mentioned this pull request Mar 10, 2020

Extend PBFT tests with protocol parameter update proposals/votes/endorsements/adoption #1514

Closed

mrBliss approved these changes Mar 10, 2020

View reviewed changes

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/BFT.hs Outdated Show resolved Hide resolved

ouroboros-consensus/ouroboros-consensus-mock/test/Test/ThreadNet/TxGen/Mock.hs Outdated Show resolved Hide resolved

mrBliss reviewed Mar 10, 2020

View reviewed changes

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch 2 times, most recently from 0c59660 to df3c264 Compare March 10, 2020 19:17

edsko reviewed Mar 11, 2020

View reviewed changes

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from df3c264 to 2926f99 Compare March 11, 2020 15:39

nfrisby requested a review from edsko March 11, 2020 16:37

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from 2926f99 to c2c0431 Compare March 12, 2020 17:15

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from c2c0431 to 1331935 Compare March 12, 2020 18:28

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from 1331935 to a0875cf Compare March 13, 2020 13:51

mrBliss reviewed Mar 16, 2020

View reviewed changes

nfrisby added 5 commits March 16, 2020 07:19

consensus: orphan NFData SlotNo

166aee3

consensus: add optional expiry slot to Mock transactions

ec5f0c1

mock: use 20s slot length in BFT tests

31b7a03

mock: fix typo in comment

b4d98e9

mock: rename HasTxs to HasMockTxs to avoid confusion with Mempool.HasTxs

e3767e2

nfrisby force-pushed the nfrisby/issue-1297-tx-ttl branch from a0875cf to e3767e2 Compare March 16, 2020 14:19

nfrisby added 2 commits March 16, 2020 07:21

mock: whitespace

a054d87

test-infra: alphabetize imports

acf47b0

iohk-bors bot merged commit 742c5a4 into master Mar 16, 2020

iohk-bors bot deleted the nfrisby/issue-1297-tx-ttl branch March 16, 2020 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional expiry slot to Mock transactions #1564

Add optional expiry slot to Mock transactions #1564

nfrisby commented Feb 3, 2020 •

edited

Loading

mrBliss Feb 4, 2020

nfrisby Feb 4, 2020 •

edited

Loading

mrBliss left a comment

mrBliss Mar 10, 2020

edsko Mar 10, 2020

mrBliss Mar 10, 2020

nfrisby Mar 10, 2020

nfrisby commented Mar 10, 2020

nfrisby commented Mar 10, 2020

edsko left a comment •

edited

Loading

nfrisby commented Mar 11, 2020

nfrisby commented Mar 12, 2020

nfrisby commented Mar 12, 2020

nfrisby commented Mar 13, 2020

nfrisby commented Mar 16, 2020

nfrisby commented Mar 16, 2020

iohk-bors bot commented Mar 16, 2020

Add optional expiry slot to Mock transactions #1564

Add optional expiry slot to Mock transactions #1564

Conversation

nfrisby commented Feb 3, 2020 • edited Loading

mrBliss Feb 4, 2020

Choose a reason for hiding this comment

nfrisby Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

mrBliss left a comment

Choose a reason for hiding this comment

mrBliss Mar 10, 2020

Choose a reason for hiding this comment

edsko Mar 10, 2020

Choose a reason for hiding this comment

mrBliss Mar 10, 2020

Choose a reason for hiding this comment

nfrisby Mar 10, 2020

Choose a reason for hiding this comment

nfrisby commented Mar 10, 2020

nfrisby commented Mar 10, 2020

edsko left a comment • edited Loading

Choose a reason for hiding this comment

nfrisby commented Mar 11, 2020

nfrisby commented Mar 12, 2020

nfrisby commented Mar 12, 2020

nfrisby commented Mar 13, 2020

nfrisby commented Mar 16, 2020

nfrisby commented Mar 16, 2020

iohk-bors bot commented Mar 16, 2020

Build succeeded

nfrisby commented Feb 3, 2020 •

edited

Loading

nfrisby Feb 4, 2020 •

edited

Loading

edsko left a comment •

edited

Loading