More intelligent transaction requests #1476

intricate · 2020-01-20T02:16:47Z

Closes #1161

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs

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

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs

dcoutts

I'm a bit concerned that we've broken the clean pattern here with serverReqTxs, and I wonder if we can keep to it, despite the extra feature we've gained.

So previously we had canRequestMoreTxs which was pure and told is if we did indeed have more txs to request, and then serverReqTxs simply did the request.

What we have now is a bit of a mish-mash. We still have a pure canRequestMoreTxs but it's now not definitive. Instead we have serverReqTxs which no longer performs the request at all. Instead it does the impure tests that were not covered by canRequestMoreTxs and returns what the request should be, or updates the state with extra txids that can be acknowledged.

My suggestion is this: introduce a new step at appropriate points in the server loop where we read the current recent txs set

recentTxIds <- atomically $ readTVar recentTxIdsVar

This is the only actual action needed. After this we can update the various bits of server state purely. Then the old pattern works again: canRequestMoreTxs is again definitive since it just looks at the updated server state, and serverReqTxs can actually request txs.

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs

dcoutts · 2020-02-18T08:59:00Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs

+        Just expTime -> pure expTime
+    currentTime <- getMonotonicTime
+    let toWait = max 0 (timeScheduledForExpiry `diffTime` currentTime)
+    threadDelay toWait


We really need to fix threadDelay on 32bit systems. Any wait over ~35min will fail, and our expiry here is an hour. Lets file a ticket so we don't forget.

That said 60min is probably 60x too long.

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs

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

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs

dcoutts · 2020-02-18T10:14:25Z

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

@@ -642,7 +643,7 @@ runThreadNetwork ThreadNetworkArgs
            , blockProduction         = Just blockProduction
            , blockFetchSize          = nodeBlockFetchSize
            , blockMatchesHeader      = nodeBlockMatchesHeader
-            , recentTxIdsExpiryThresh = Time (secondsToDiffTime (60 * 60)) -- TODO
+            , recentTxIdsExpiryThresh = RecentTxIds.ExpiryThreshold (secondsToDiffTime (60 * 60))


Can be just RecentTxIds.ExpiryThreshold 60.0 I think. There's a Num instance for DiffTime and the interpretation is seconds.

Would it be obvious to someone reading the code that the value is interpreted in seconds? If so, I can make the change.

dcoutts · 2020-02-18T10:17:03Z

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs

@@ -271,7 +273,7 @@ mkNodeArgs registry cfg initState tracers btime chainDB isProducer = NodeArgs
    , blockProduction
    , blockFetchSize          = nodeBlockFetchSize
    , blockMatchesHeader      = nodeBlockMatchesHeader
-    , recentTxIdsExpiryThresh = Time (secondsToDiffTime 3600)
+    , recentTxIdsExpiryThresh = RecentTxIds.ExpiryThreshold (secondsToDiffTime (60 * 60))


60s will be enough. Ideally I'd make it scale automatically with the expected block time for the protocol, and use 3x the block time.

Also, we don't need secondsToDiffTime here, since the Num.fromIntegral for DiffTime uses seconds.

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs

nix/.stack.nix/ouroboros-network.nix

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs

mrBliss · 2020-02-20T12:19:09Z

To test this, I have the following approach in mind:

Modify the protocol tracers of the ThreadNet tests to trace all requests for transactions. Also trace transactions added and expired to/from the RecentTxIds. In prop_general, check for each node (by looking at its traces) that it never requested a transaction that is still in its RecentTxIds.

ouroboros-network/src/Ouroboros/Network/Util/Orphans.hs

mrBliss · 2020-02-25T14:57:05Z

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs

+      ![txid]
+      -- ^ Transaction IDs that have been inserted into the 'RecentTxIds'.
+      !Time
+      -- ^ The time at which the transactions were added to the 'RecentTxIds'.
+  | TraceRecentTxIdsExpired
+      ![(txid, Time)]
+      -- ^ Transaction IDs, along with their expiration times, that have been
+      -- expired from the 'RecentTxIds'.


See #1688 (comment).

If there's the potential to hold onto memory memory, then some forcing might not be crazy.

On the other hand: we have #1386 open for that.

mrBliss · 2020-02-25T15:02:05Z

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/ThreadNet/Network.hs

+  , nodeEventsTxRequests  :: ev ([GenTxId blk], RecentTxIds (GenTxId blk))
+    -- ^ every request for transactions made by the node-to-node
+    -- 'TxSubmission' mini-protocol along with the 'RecentTxIds' at the time
+    -- each request was made.


So we store the RecentTxIds each time a request for txs is sent. I suppose it will be fine, but can you check that the memory usage while running the tests doesn't increase by too much? And can you check what the impact on the run time is? (Use the same seed to run the tests a few times with and without the new checks.) The tests are already slow enough 🙂

mrBliss · 2020-02-25T15:04:22Z

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

+    -- i.e. Transactions that are in a node's 'RecentTxIds' should not be
+    -- requested from a peer.
+    prop_no_txs_repeatedly_requested :: Property
+    prop_no_txs_repeatedly_requested =


Very nice! This is exactly what I meant (even simpler 👍).

Co-authored-by: Thomas Winant <[email protected]>

intricate · 2020-03-05T01:19:27Z

Closed in favour of #1749 which implements the new plan.

intricate requested a review from mrBliss January 20, 2020 02:16

intricate self-assigned this Jan 20, 2020

intricate commented Jan 20, 2020

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Node.hs Outdated Show resolved Hide resolved

intricate force-pushed the intricate/1161 branch from 4c3cdff to da30a03 Compare January 20, 2020 02:20

intricate commented Jan 20, 2020

View reviewed changes

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs Outdated Show resolved Hide resolved

intricate force-pushed the intricate/1161 branch from da30a03 to 3ee99f6 Compare January 20, 2020 02:37

mrBliss reviewed Jan 22, 2020

View reviewed changes

intricate force-pushed the intricate/1161 branch from 3ee99f6 to 2a5baee Compare January 27, 2020 15:43

intricate force-pushed the intricate/1161 branch from 2a5baee to b3576f4 Compare February 3, 2020 20:10

intricate force-pushed the intricate/1161 branch 4 times, most recently from 3058fa4 to 8d49781 Compare February 11, 2020 17:54

intricate requested a review from dcoutts February 11, 2020 18:21

mrBliss self-requested a review February 12, 2020 16:44

mrBliss mentioned this pull request Feb 14, 2020

Avoid space leaks in TxSubmission.Inbound #1645

Closed

mrBliss marked this pull request as ready for review February 17, 2020 09:12

coot reviewed Feb 18, 2020

View reviewed changes

ouroboros-network/src/Ouroboros/Network/RecentTxIds.hs Outdated Show resolved Hide resolved

dcoutts requested changes Feb 18, 2020

View reviewed changes

coot reviewed Feb 18, 2020

View reviewed changes

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs Outdated Show resolved Hide resolved

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound.hs Outdated Show resolved Hide resolved

intricate force-pushed the intricate/1161 branch 2 times, most recently from 8d77e16 to f8f1a68 Compare February 20, 2020 02:08

intricate requested a review from dcoutts February 20, 2020 02:08

intricate force-pushed the intricate/1161 branch from f8f1a68 to 039066b Compare February 20, 2020 03:23

mrBliss reviewed Feb 20, 2020

View reviewed changes

intricate force-pushed the intricate/1161 branch 2 times, most recently from 12d0bdf to 4dd484c Compare February 24, 2020 20:19

intricate requested a review from mrBliss February 24, 2020 20:19

intricate added the byron Required for a Byron mainnet: replace the old core nodes with cardano-node label Feb 24, 2020

intricate added consensus issues related to ouroboros-consensus networking labels Feb 24, 2020

intricate force-pushed the intricate/1161 branch 4 times, most recently from eace5fd to 18b7997 Compare February 25, 2020 05:57

mrBliss reviewed Feb 25, 2020

View reviewed changes

intricate force-pushed the intricate/1161 branch from 18b7997 to 45a709d Compare February 25, 2020 17:23

intricate and others added 8 commits February 25, 2020 13:32

Implement RecentTxIds data structure

f03e257

More intelligent transaction requests

18644c6

Co-authored-by: Thomas Winant <[email protected]>

Rename TxSubmission to LocalTxSubmissionServer

5787843

LocalTxSubmissionServer: add transactions to RecentTxIds

bf8b421

Add issue number to TODO comment in txSubmissionInbound

b8e6c3f

Extend prop_general to test RecentTxIds functionality

0dc72d4

Add tracing for events related to RecentTxIds

f75793e

Add NoUnexpectedThunks instance for RecentTxIds

97d07d0

intricate force-pushed the intricate/1161 branch from 45a709d to 97d07d0 Compare February 25, 2020 19:33

This was referenced Mar 4, 2020

More intelligent transaction requests #1161

Closed

Intelligently request the same transaction from multiple nodes #1743

Closed

intricate closed this Mar 5, 2020

intricate deleted the intricate/1161 branch April 1, 2020 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More intelligent transaction requests #1476

More intelligent transaction requests #1476

intricate commented Jan 20, 2020 •

edited

Loading

dcoutts left a comment

dcoutts Feb 18, 2020

dcoutts Feb 18, 2020

intricate Feb 25, 2020

dcoutts Feb 18, 2020

mrBliss commented Feb 20, 2020

mrBliss Feb 25, 2020

mrBliss Feb 25, 2020

mrBliss Feb 25, 2020

intricate commented Mar 5, 2020

More intelligent transaction requests #1476

More intelligent transaction requests #1476

Conversation

intricate commented Jan 20, 2020 • edited Loading

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Feb 18, 2020

Choose a reason for hiding this comment

dcoutts Feb 18, 2020

Choose a reason for hiding this comment

intricate Feb 25, 2020

Choose a reason for hiding this comment

dcoutts Feb 18, 2020

Choose a reason for hiding this comment

mrBliss commented Feb 20, 2020

mrBliss Feb 25, 2020

Choose a reason for hiding this comment

mrBliss Feb 25, 2020

Choose a reason for hiding this comment

mrBliss Feb 25, 2020

Choose a reason for hiding this comment

intricate commented Mar 5, 2020

intricate commented Jan 20, 2020 •

edited

Loading