search

XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.48k stars 1.45k forks source link

Improve transaction relay logic #4985

Open ximinez opened 2 months ago

ximinez commented 2 months ago

High Level Overview of Change

This PR, if merged, will improve transaction relay logic around a few edge cases.

(I'll write a single commit message later, but before this PR is squashed and merged.)

Context of Change

A few months ago, while examining some of the issues around the 2.0.0 release, and auditing transaction relay code, I identified a few areas with potential for improvement.

Type of Change

Before / After

This PR is divided into four mostly independent changes.

  1. "Decrease shouldRelay limit to 30s." Pretty self-explanatory. Currently, the limit is 5 minutes, by which point the HashRouter entry could have expired, making this transaction look brand new (and thus causing it to be relayed back to peers which have sent it to us recently).
  2. "Give a transaction more chances to be retried." Will put a transaction into LedgerMaster's held transactions if the transaction gets a ter, tel, or tef result. Old behavior was just ter.
    • Additionally, to prevent a transaction from being repeatedly held indefinitely, it must meet some extra conditions. (Documented in a comment in the code.)
  3. "Pop all transactions with sequential sequences, or tickets." When a transaction is processed successfully, currently, one held transaction for the same account (if any) will be popped out of the held transactions list, and queued up for the next transaction batch. This change pops all transactions for the account, but only if they have sequential sequences (for non-ticket transactions) or use a ticket. This issue was identified from interactions with @mtrippled's #4504, which was merged, but unfortunately reverted later by #4852. When the batches were spaced out, it could potentially take a very long time for a large number of held transactions for an account to get processed through. However, whether batched or not, this change will help get held transactions cleared out, particularly if a missing earlier transaction is what held them up.
  4. "Process held transactions through existing NetworkOPs batching." In the current processing, at the end of each consensus round, all held transactions are directly applied to the open ledger, then the held list is reset. This bypasses all of the logic in NetworkOPs::apply which, among other things, broadcasts successful transactions to peers. This means that the transaction may not get broadcast to peers for a really long time (5 minutes in the current implementation, or 30 seconds with this first commit). If the node is a bottleneck (either due to network configuration, or because the transaction was submitted locally), the transaction may not be seen by any other nodes or validators before it expires or causes other problems.
codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 86.95652% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 70.9%. Comparing base (5aa1106) to head (3863b01).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4985/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ Coverage Diff @@ ## develop #4985 +/- ## ======================================= Coverage 70.9% 70.9% ======================================= Files 796 796 Lines 66792 66851 +59 Branches 11002 11003 +1 ======================================= + Hits 47379 47429 +50 - Misses 19413 19422 +9 ``` | [Files](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [src/ripple/app/ledger/LocalTxs.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fledger%2FLocalTxs.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbGVkZ2VyL0xvY2FsVHhzLmg=) | `100.0% <ø> (ø)` | | | [src/ripple/app/ledger/impl/LedgerMaster.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fledger%2Fimpl%2FLedgerMaster.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbGVkZ2VyL2ltcGwvTGVkZ2VyTWFzdGVyLmNwcA==) | `40.0% <100.0%> (-<0.1%)` | :arrow_down: | | [src/ripple/app/ledger/impl/LocalTxs.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fledger%2Fimpl%2FLocalTxs.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbGVkZ2VyL2ltcGwvTG9jYWxUeHMuY3Bw) | `100.0% <100.0%> (ø)` | | | [src/ripple/app/main/Application.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmain%2FApplication.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWFpbi9BcHBsaWNhdGlvbi5jcHA=) | `63.1% <100.0%> (+<0.1%)` | :arrow_up: | | [src/ripple/app/misc/CanonicalTXSet.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2FCanonicalTXSet.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9DYW5vbmljYWxUWFNldC5jcHA=) | `100.0% <100.0%> (ø)` | | | [src/ripple/app/misc/HashRouter.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2FHashRouter.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9IYXNoUm91dGVyLmNwcA==) | `100.0% <100.0%> (ø)` | | | [src/ripple/app/misc/HashRouter.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2FHashRouter.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9IYXNoUm91dGVyLmg=) | `100.0% <100.0%> (ø)` | | | [src/ripple/app/misc/NetworkOPs.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2FNetworkOPs.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9OZXR3b3JrT1BzLmg=) | `100.0% <ø> (ø)` | | | [src/ripple/app/misc/NetworkOPs.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2FNetworkOPs.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9OZXR3b3JrT1BzLmNwcA==) | `66.2% <82.6%> (+0.5%)` | :arrow_up: | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/4985/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4985/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4985?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)