Bolt12/coot/tx submission by bolt12 · Pull Request #4928 · IntersectMBO/ouroboros-network

bolt12 · 2024-08-12T14:14:40Z

No description provided.

Allow monadic action when trying to pipeline more messages, while collecting responses.

* `prop_makeDecisions_receivedTxIds` mix up `makeDecisions` and `receivedTxIds` * `prop_makeDecisions_collectTxIds` mix up `makeDecisions` and `collectTxIds`

Export everything from the `Ouroboros.Network.TxSubmission.Inbound` module.

- Factors out common type definitions and their generators and properties to a Common.hs file. - Adds a TxSubmissionV2 file with the boilerplate to run the new, more accurate submission that uses the V2 version of TxSubmission protocol

Refactored SimResult name to not clash with IOSim's

- Updated txSubmissionV2 test - Fixed TODO about passing an STM action inside receivedTxIds - Fixed usage of partial function `(!)` - Fixed wrong usage of MVars in decisionLogicThread that lead to deadlock.

There was a race condition between the `decisionLogicThread` producing the right policy and inbound server picking up the most up to date policy. This would lead to the inbound side issuing a blocking request when the client was awaiting for a non-blocking request. This blocking request isn't wrong considering the policy it was used, it is a legit decision that's made which leads to the inbound server issuing a blocking request but because we have received a txid in the meantime and didn't manage to read it soon enough we didn't create a more important decision. The fix involved being aware of how many requests for txs we have in flight and not generate "low priority" policies. `hasTxIdsToAcknowledge` is not used anywhere in the code so it is removed. `filterActivePeers` is improved by making its decision logic more closed to `pickTxsToDownload`. `filterActivePeers` test is fixed, since it doesn't hold under the new logic: `filterActivePeers` will not compute a decision for peers which have `requestedTxIdsInflight` and `makeDecisions` computes non-empty decisions for peers with no `requestedTxIdsInflight`. So: 1. "The set of active peers is a superset of peers for which a decision was made" this is not true since it is possible that a non active peer has a legitimate decision, but due to our race-condition protection condition we just don't generate it. 2. "The set of active peer which can acknowledge txids is a subset of peers for which a decision was made" this is removed since hasTxIdsToAcknowledge function is removed 3. "Decisions made from the results of `filterActivePeers` is the same as from the original set" this isn't true because of what I said above So I refactored the test to check that the number of filtered decisions is a subset of the total number of decisions, which I believe to be a more accurate test for the current logic

- Adds tx submission diffusion testnet test This test checks that even in the presence of a node that keeps disconnecting, but eventually stays online, we manage to learn about all its transactions.

Module structure needs to be reorganised to have just one debug tracer.

Use `IOSim` API. `evaluateTrace` from `Test.Ouroboros.Network.LedgerPeers` has the annoying property that once the trace was evaluated in won't show the trace again, which makes it hard to work with `cabal repl`. Refactored `checkMempools` to improve readablity. Should be squashed onto `c9d45673ca New txSubmissionV2 simulation`

coot

Some remarks on the PR.

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/TxSubmission/TxSubmissionV2.hs

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

coot · 2024-09-13T12:32:23Z

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

+defaultTxDecisionPolicy :: TxDecisionPolicy
+defaultTxDecisionPolicy =
+  TxDecisionPolicy {
+    maxNumTxIdsToRequest   = 1,
+    maxUnacknowledgedTxIds = 2,
+    txsSizeInflightPerPeer = 2,
+    maxTxsSizeInflight     = maxBound,
+    txInflightMultiplicity = 2
+  }


These numbers are far from production configuration.

Suggested change

defaultTxDecisionPolicy :: TxDecisionPolicy

defaultTxDecisionPolicy =

TxDecisionPolicy {

maxNumTxIdsToRequest = 1,

maxUnacknowledgedTxIds = 2,

txsSizeInflightPerPeer = 2,

maxTxsSizeInflight = maxBound,

txInflightMultiplicity = 2

}

defaultTxDecisionPolicy :: TxDecisionPolicy

defaultTxDecisionPolicy =

TxDecisionPolicy {

maxNumTxIdsToRequest = 3,

maxUnacknowledgedTxIds = 10,

txsSizeInflightPerPeer = error "TODO", -- currently we download at most 2 tx-s in `MsgReplytTxs`.

maxTxsSizeInflight = error "TODO", -- ?

txInflightMultiplicity = 1

}

https://input-output-rnd.slack.com/archives/C050LAMLL9E/p1726230989272239

coot · 2024-09-13T14:15:43Z

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

+      ( 0
+      , 0
+      , []
+      , RefCountDiff Map.empty
+      , ps
+      )
  where


Suggested change

( 0

, 0

, []

, RefCountDiff Map.empty

, ps

)

where

emptyAck

where

emptyAck = (0, 0, [], RefCoundDiff Map.empty, ps)

coot · 2024-09-13T14:25:40Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/TxSubmission/TxSubmissionV2.hs

+        Left err -> counterexample (ppTrace trace)
+                 $ counterexample (show err)
+                 $ property False


indentation

Suggested change

Left err -> counterexample (ppTrace trace)

$ counterexample (show err)

$ property False

Left err -> counterexample (ppTrace trace)

$ counterexample (show err)

$ property False

coot · 2024-09-13T14:34:13Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/TxSubmission/TxSubmissionV2.hs

+                      )
+                      Map.empty
+                      inmp
+           in resultRepeatedValidTxs === maxRepeatedValidTxs


I think this only works because there's no attenuation. The logic here counts how many times a tx is downloaded over the whole simulation rather than from how many peers it is being downloaded from at the same time. The latter could be done using tx-submission message traces, e.g. MsgRequestTxs and MsgReplyTxs. Each tx is in flight for some time between sending MsgRequestTxs and getting MsgReplyTxs back. Having a list of (startTime, endTime) for each tx's we'd need to check that we never have more than the configured number of them at each time. So we'd need a function:

maxOverlapped :: [(Time, Time)] -> Int

which counts the maximal number of overlapping intervals.

Yes, that's true, in Diffusion Sim we have the same test but with attenuation.

bolt12 · 2024-09-18T11:49:07Z

closed due to it being merged into #4887

bolt12 force-pushed the bolt12/coot/tx-submission branch 4 times, most recently from f434785 to 91304d1 Compare August 14, 2024 10:30

bolt12 force-pushed the bolt12/coot/tx-submission branch from da7ff95 to 97ca973 Compare August 23, 2024 12:22

bolt12 force-pushed the bolt12/coot/tx-submission branch 2 times, most recently from 8504d49 to 9e6e0f4 Compare September 16, 2024 10:40

bolt12 marked this pull request as ready for review September 16, 2024 10:41

bolt12 requested a review from a team as a code owner September 16, 2024 10:41

bolt12 self-assigned this Sep 16, 2024

bolt12 requested a review from coot September 16, 2024 10:46

coot added 9 commits September 16, 2024 21:23

tx-submission: newtype wrappers NumTxIdsTo{Ack,Req}

ae13acc

tx-submission: PeerTxState & SharedTxState

03c00a7

tx-submission: decision logic

832ea61

tx-submission: generalised CollectPipelined

5b822c2

Allow monadic action when trying to pipeline more messages, while collecting responses.

tx-submission: registry

4d78201

tx-submission: additional makeDecisions properties

2783b60

* `prop_makeDecisions_receivedTxIds` mix up `makeDecisions` and `receivedTxIds` * `prop_makeDecisions_collectTxIds` mix up `makeDecisions` and `collectTxIds`

tx-submission: inbound peer using tx-submission decision logic

c6b9ab4

ouroboros-network: tx-submission module structure

992a3ec

Export everything from the `Ouroboros.Network.TxSubmission.Inbound` module.

tx-submission: debug tracer for SharedTxState

228aa28

coot force-pushed the coot/tx-submission branch from 992a7de to 228aa28 Compare September 16, 2024 19:26

coot requested review from a team and newhoggy as code owners September 16, 2024 19:26

bolt12 and others added 7 commits September 16, 2024 21:28

Refactor TxSubmission files and added V2 sim

12915a4

- Factors out common type definitions and their generators and properties to a Common.hs file. - Adds a TxSubmissionV2 file with the boilerplate to run the new, more accurate submission that uses the V2 version of TxSubmission protocol

Fixed ArbTxDecisionPolicy generator

c45292f

Refactored SimResult name to not clash with IOSim's

New txSubmissionV2 simulation

22a0def

- Updated txSubmissionV2 test - Fixed TODO about passing an STM action inside receivedTxIds - Fixed usage of partial function `(!)` - Fixed wrong usage of MVars in decisionLogicThread that lead to deadlock.

Added test that checks tx multiplicities

6a452f3

Integrates txSubmissionV2 in testnet diffusion sim

23be333

- Adds tx submission diffusion testnet test This test checks that even in the presence of a node that keeps disconnecting, but eventually stays online, we manage to learn about all its transactions.

fixup! Fix race condition when producing the right policy

fda2141

coot removed request for a team and newhoggy September 16, 2024 19:36

coot added 3 commits September 16, 2024 21:41

tx-submission: added DebugTxLogic tracer

22a7bc2

Module structure needs to be reorganised to have just one debug tracer.

tx-submission: label TVars in tests

2541e95

coot force-pushed the bolt12/coot/tx-submission branch from 9e6e0f4 to 99097c7 Compare September 16, 2024 19:42

coot reviewed Sep 17, 2024

View reviewed changes

coot force-pushed the coot/tx-submission branch 5 times, most recently from 76c0d13 to 11d833b Compare September 18, 2024 11:26

bolt12 closed this Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bolt12/coot/tx submission#4928

Bolt12/coot/tx submission#4928
bolt12 wants to merge 19 commits intocoot/tx-submissionfrom
bolt12/coot/tx-submission

bolt12 commented Aug 12, 2024

Uh oh!

coot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coot Sep 13, 2024

Uh oh!

coot Sep 13, 2024

Uh oh!

coot Sep 13, 2024

Uh oh!

coot Sep 13, 2024

Uh oh!

coot Sep 13, 2024

Uh oh!

bolt12 Sep 17, 2024

Uh oh!

bolt12 commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-      ( 0
-      , 0
-      , []
-      , RefCountDiff Map.empty
-      , ps
-      )
-  where
+      emptyAck
+  where
+    emptyAck = (0, 0, [], RefCoundDiff Map.empty, ps)

Conversation

bolt12 commented Aug 12, 2024

Uh oh!

coot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coot Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

coot Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

coot Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

coot Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

coot Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

bolt12 Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

bolt12 commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants