Mempool.removeTxs should not use the fast path #1565
Assignees
Milestone
Comments
mrBliss
added
bug
|
Also see #1505 (comment) |
mrBliss
added a commit
that referenced
this issue
Feb 6, 2020
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. Moreover, using the new transaction type reveals a bug that was not visible with the previous transaction type, i.e., #1565.
mrBliss
added a commit
that referenced
this issue
Feb 6, 2020
mrBliss
added a commit
that referenced
this issue
Feb 7, 2020
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. Moreover, using the new transaction type reveals a bug that was not visible with the previous transaction type, i.e., #1565.
mrBliss
added a commit
that referenced
this issue
Feb 7, 2020
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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. Moreover, using the new transaction type reveals a bug that was not visible with the previous transaction type, i.e., #1565.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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. Moreover, using the new transaction type reveals a bug that was not visible with the previous transaction type, i.e., #1565.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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. Moreover, using the new transaction type reveals a bug that was not visible with the previous transaction type, i.e., #1565.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
mrBliss
added a commit
that referenced
this issue
Feb 10, 2020
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 (#1565) 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.
iohk-bors bot
added a commit
that referenced
this issue
Feb 10, 2020
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]>
iohk-bors bot
added a commit
that referenced
this issue
Feb 10, 2020
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]>
iohk-bors bot
added a commit
that referenced
this issue
Mar 2, 2020
1506: Fix cloneBlockchainTime r=nfrisby a=nfrisby Fixes #1489. Fixes #1524. Fixing Issue #1489 (let nodes lead when they join) and letting k vary in the range [2 .. 10] since the dual-ledger tests do that now revealed several Issues. Issues in the library, not just the test infrastructure: * Issue #1505 -- `removeTxs` cannot use the fast path when validating after removing or else we might have dangling tx inputs references. Was fixed by #1565. * Issue #1511, bullet 1 (closed) -- The `Empty` cases in `prevPointAndBlockNo` were wrong. Recent PRs have addressed this: #1544 and #1589. * Issue #1543 (closed) -- A bracket in `registeredStream` was spoiled by an interruptible operation. Thomas' PR re-designed the vulnerability away. I think this is unrelated to the other changes; it was lurking and happened to pop up here just because I've been running hundreds of thousands of tests. Issues only in the test infrastructure: * Issue #1489: let nodes lead when they join. This bug slipped in recently, when I added cloning of `BlockchainTime`s as part of the restarting/rekeying loop in the test infrastructure. * _PBFT reference simulator_. (Was masked by 1489.) Model competing 1-block chains in Ref.PBFT and use its results where applicable instead of the .Expectations module. Check that the PBFT threadnet and Ref.PBFT.simulate results agree on `Ref.Nominal` slots. The Ref.PBFT module had been making assumptions that were accurate given the guards in RealPBFT generators, given the `k >= n` regime. But outside of that regime, when the security parameter exceeds the node count, it wasn't enough. Also, it couldn't be compared against the PBFT threadnet because of those assumptions. * _PBFT reference simulator_. (Cascade of above.) Add `definitelyEnoughBlocks` to confirm the "at least k blocks in 2k slots" invariant in `genRealPBFTNodeJoinPlan`. The existing guards in the RealPBFT generators are intentionally insufficient by themselves; this way we can optimize them to avoid O(n^2) complexity without risking divergence from the `suchThat`s. * _Origin corner-case_. (Was masked by 1489.) Discard DualPBFT tests that forge in slot 0. The current `cardano-ledger-specs` doesn't allow for that. My hypothesis is that `cvsLastSlot` would need to be able to represent origin. * _Dlg cert tx_. Adjust `genNodeRekeys` to respect "If node X rekeys in slot S and Y leads slot S+1, then either the topology must connect X and Y directly, or Y must join before slot S." * _Dlg cert tx_. (Was (statistically?) masked by relatively large k.) Add `rekeyOracle` and use it to determine which epoch number to record in the dlg cert. The correct value depends on which block the dlg cert tx will end up in, not when we first add it to our mempool. * _Dlg cert tx_. (Was (statistically?) masked by relatively large k.) Add the dlg cert tx to the mempool each time the ledger changes. Co-authored-by: Nicolas Frisby <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See #1505 (comment) for a detailed bug report.
The text was updated successfully, but these errors were encountered: