Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TX Submission Logic #4887

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

TX Submission Logic #4887

wants to merge 26 commits into from

Conversation

coot
Copy link
Contributor

@coot coot commented Jun 4, 2024

Description

This is a draft PR (work in progress) which introduces tx-submission logic responsible for choosing from which peer to download a tx.

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@coot coot added the tx-submission Issues related to tx-submission protocol label Jun 4, 2024
@coot coot linked an issue Jun 4, 2024 that may be closed by this pull request
3 tasks
@coot coot force-pushed the coot/tx-submission branch 4 times, most recently from 4bb6b38 to 8b53f24 Compare September 18, 2024 05:12
coot and others added 19 commits September 18, 2024 13:13
Allow monadic action when trying to pipeline more messages, while
collecting responses.
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
Add `max_TX_SIZE` which is shared between

* `defaultTxDecisionPolicy`, and
* `txSubmissionProtocolLimits`
- 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`
This allows us to have just one tracer for tx-submission decision logic.
* Put `TxLogic` tests in a separate module, and common API in `Types`.
* Fixed tasty test hierarchy.
* Renamed `TxSubmissionV{1,2}` as `AppV{1,2}`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tx-submission Issues related to tx-submission protocol
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

tx-submission decision logic for the inbound side
2 participants