Improve transaction relay logic

[ximinez]

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Tests (you added tests for code that already exists, or your new feature included in this PR)

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 Apply transaction batches in periodic intervals. #4504, which was merged, but unfortunately reverted later by Revert "Apply transaction batches in periodic intervals (#4504)" #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]

Codecov Report

Attention: Patch coverage is 87.60331% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (c1c2b5b) to head (93b9a6d).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/misc/NetworkOPs.cpp	82.6%	15 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #4985   +/-   ##
=======================================
  Coverage     78.1%   78.1%           
=======================================
  Files          790     790           
  Lines        67908   67989   +81     
  Branches      8228    8234    +6     
=======================================
+ Hits         53024   53099   +75     
- Misses       14884   14890    +6     

Files with missing lines	Coverage Δ
src/xrpld/app/ledger/LocalTxs.h	100.0% <ø> (ø)
src/xrpld/app/ledger/detail/LedgerMaster.cpp	43.9% <100.0%> (-0.1%)	⬇️
src/xrpld/app/ledger/detail/LocalTxs.cpp	100.0% <100.0%> (ø)
src/xrpld/app/main/Application.cpp	69.2% <100.0%> (-0.1%)	⬇️
src/xrpld/app/misc/CanonicalTXSet.cpp	100.0% <100.0%> (ø)
src/xrpld/app/misc/HashRouter.cpp	100.0% <100.0%> (ø)
src/xrpld/app/misc/HashRouter.h	100.0% <100.0%> (ø)
src/xrpld/app/misc/NetworkOPs.h	100.0% <ø> (ø)
src/xrpld/app/misc/Transaction.h	100.0% <ø> (ø)
src/xrpld/app/misc/NetworkOPs.cpp	69.6% <82.6%> (+0.4%)	⬆️

... and 3 files with indirect coverage changes

[scottschurr]

Very nice! I really appreciate the way the commits were divided up. It made the code review much easier.

I mostly left complementary comments. But there's one bool that I suspect needs to be changed to a std::atomic<bool>. See what you think...

[scottschurr]

Nice! This takes full advantage of the "unusual" sort order of SeqProxy, that all sequence numbers sort in front of all tickets.

[scottschurr]

I was initially worried that this while loop might submit_held() a boatload of transactions. But the TxQ defaults maximumTxnPerAccount to 10. So the largest number of times this loop could run (ordinarily) would be 10. That seems reasonable, and a good way to clear out an account that has a lot of transactions queued.

This new characteristic may be worth pointing out to the performance folks, in case they want to stress it.

[ximinez]

To clarify, this loop doesn't do anything with the TxQ. This is reading the list of held transactions in LedgerMaster. So, AFAIK, there is no limit. However, the only way for a tx to get into that list is in this same function (see calls to addHeldTransaction), and only transactions received directly from RPC or from peers get to this function. That said, a malicious client or peer could probably get a bunch of transactions held (this is true without these changes). Thus, it might make sense to add a limit to mitigate some kinds of attacks.

Also, submit_held is a std::vector. Anything added to it will be processed in the next batch of transactions.

[scottschurr]

This is a great way to minimize processing while the lock is held. Good spotting!

[ximinez]

Thanks!

[scottschurr]

Splitting the validation and canonicalization part of processTransaction into this other method was a good idea.

[ximinez]

Thanks!

[scottschurr]

I looked into this call to Transaction::getApplying(). The bool being read is neither protected by a lock nor atomic. I think we need to change the Transaction::mApplying member variable into a std::atomic<bool>, since the bool is being accessed across threads.

This problem was present before your change. But please fix it while we're thinking about it.

[ximinez]

I think this was done intentionally because it avoids the overhead of taking a lock, and the consequences of a race condition are nearly-zero.

1. Most significantly, almost all accesses of the *Applying functions are under NetworkOPsImp's lock. The exceptions are
   1. The new processTransactionSet function.
   2. The popAcctTransaction loop we were just talking about.
2. If there is a race condition, and getApplying returns false when it should be true, the transaction will be processed again. Not that big a deal if it's a rare one-off. Most of the time, it'll get tefALREADY or tefPAST_SEQ.
3. On the flip side, if it returns true, when it should be false, then the transaction must have been attempted recently, so no big deal if it doesn't immediately get tried right away.
4. If there's a race between setApplying and clearApplying, and the flag ends up set, then a batch is about to try to process the transaction and will call clearApplying later. If it ends up cleared, then it might get attempted again later as is the case with item 2.

Alllll that said, I don't think it would hurt to rewrite those two lock exceptions I pointed out to ensure that the lock is taken for the function calls.

[vlntb]

If this optimistic locking approach doesn't create any harmful side effects, as you suggest @ximinez , it makes sense to keep the current approach. But let's put your explanation in a comment in the code to preserve it.

[scottschurr]

Similar to above, these 5 lines are not hit by the unit tests. 🤷

[ximinez]

It should be impossible to get to this line. Note the block above explains that I don't think the check is necessary, and has an assert on the same validity at the end.

    // NOTE eahennis - I think this check is redundant,
    // but I'm not 100% sure yet.
    // If so, only cost is looking up HashRouter flags.
    auto const view = m_ledgerMaster.getCurrentLedger();
    auto const [validity, reason] = checkValidity(
        app_.getHashRouter(),
        *transaction->getSTransaction(),
        view->rules(),
        app_.config());
    assert(validity == Validity::Valid);

[scottschurr]

Interesting. According to my local code coverage these four lines are never hit. I know there are a few places in the unit tests that produce corrupted signatures. They must be handled elsewhere. Just noticing, no need to address this in this pull request.

[ximinez]

Yeah, this one isn't documented as explicitly, but the idea is that it should also be impossible to get to this point with a bad signature.

[scottschurr]

Does it make sense to reserve space in mTransactions? Consider

mTransactions.reserve(mTransactions.size() + transactions.size());

[ximinez]

Yes it does!

[Bronek]

Partial review

[Bronek]

Please add to this file

#include <chrono>

[Bronek]

The error message below says something different; is this meant to be 8 or 12 ?

Also, given this is a free function, it should be easy to add to unit tests, including test that an exception is thrown.

[ximinez]

The error message below says something different; is this meant to be 8 or 12 ?

This looks like a copy/paste error. Unfortunately, I don't remember which direction. LOL.

[Bronek]

This is confusing. On the one hand, this comment and the code implies that we should best rely on hardcoded values, on the other it is read from configuration, but the new configuration values are not documented in cfg/rippled-example.cfg.

Perhaps it would be better to 1. remove default constructor 2. remove default values from members and move them to setup_HashRouter, adding comment here about where the default values are 3. make the data members const and 4. add hashrouter section and appropriate documentation to cfg/rippled-example.cfg

I do not insist this is better but it will be probably easier to understand what's going on : better cohesion re. parsing configuration and populating this structure, immutable data members, documented config in cfg/rippled-example.cfg

Alternatively (perhaps even better), keep the default values here, give up on reading this from configuration and remove setup_HashRouter altogether. If network-wide action is needed to update these values, such action could be in principle "deploy new binary" rather than "agree with everyone to change the configuration", where deploying new binary has the added benefit that, in principle, everyone can see which validator is running which binary version.

[ximinez]

This is a pretty common pattern throughout rippled. For example FeeSetup and setup_FeeVote. The difference is that FeeSetup is documented. It's done this way with default values so that the structures can be used in, for example, unit tests using either the default values, or custom values without needing the overhead of having to populate a Config object. It's also handy to be able to look in one place to see what the defaults are. Most of the objects that use this pattern are singletons that are created on startup, so it's not like anybody is going to forget to call the setup_ function.

This isn't the only undocumented configuration feature in rippled. Those exist intentionally, because in some situations, it can be a lot faster to communicate with operators (especially UNL operators) and get them to update their config than it would be to roll out a new version, even if it's a hotfix.

Based on your feedback, I did update this comment to reflect

1. The fact that it's undocumented.
2. That setup_HashRouter processes the config.

[Bronek]

Thank you, that works

[Bronek]

Can we have a comment here why relayTime value is used for holdTime parameter ? Also, it appears to be the only use of the Entry::shouldRelay function, perhaps this parameter needs to be renamed to relayTime or retryTime ?

[ximinez]

Can we have a comment here why relayTime value is used for holdTime parameter ? Also, it appears to be the only use of the Entry::shouldRelay function, perhaps this parameter needs to be renamed to relayTime or retryTime ?

The parameter in s.shouldRelay is named holdTime because that's the value that was used before I introduced the shorter relay retry cutoff. I'll rename it.

[Bronek]

please use XRPL_ASSERT

[ximinez]

please use XRPL_ASSERT

Good catch. Thanks.