XRPLF / rippled

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

tx method: better handling of cached transactions that can never be validated (Version: v1.7.0-b10) #3727

Open mDuo13 opened 3 years ago

mDuo13 commented 3 years ago

Summary

When you use the tx method on a transaction that's been cached but does not appear in any ledger, the response has a special case format that is undocumented and unhelpful.

We should add the searched_all response field to this case if you provided the min_ledger and max_ledger fields (added in 1.5.0), or remove this case so that it results in a txnNotFound error response that already has searched_all. There are some other improvements that could be helpful as well, but that's the low-hanging fruit.

Motivation

When looking up a transaction, you really would like to be able to determine its final outcome. That means separating the following three cases:

  1. The transaction is found in a validated ledger with immutable results.
  2. The transaction is not in a validated ledger but may become validated in the future.
  3. The transaction is not in any validated ledger and never will be.

The difference between cases (2) and (3) can be difficult to ascertain. Generally speaking, you need to determine the lower and upper bounds for what ledger the transaction is in, then you need to confirm that you have checked all of those ledgers and the transaction is in none of them.

The min_ledger and max_ledger fields help you confirm that all the ledgers were checked if you got a txnNotFound response, but they do not help if the transaction was found, but not in any known ledger. In that case, tx returns a result such as the following:

{
  "id": 1,
  "result": {
    "Account": "rKBi32xVtepHSbtG8SciPZR7NyH7JtuUkK",
    "Amount": "10000",
    "Destination": "rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe",
    "Fee": "12",
    "Flags": 2147483648,
    "LastLedgerSequence": 14069654,
    "Sequence": 14069567,
    "SigningPubKey": "038F25A3875580D145179B81CE738BAE553AF6792A33628869E8F88180C307903B",
    "TransactionType": "Payment",
    "TxnSignature": "3045022100D9BEA5BD84244FD4EEAE23BFAE111D7BD8F5DE8CA30F47E9BA0D8385A00B5C4A022011AB58E7C4C7885A5417FCC8BABBF4A9F832DF63FA2E444E323D975BE5DBAB59",
    "hash": "17C3E09F3D90D311CE4944D2BF42E8ACFC0C6205147487F2EB2BD024ECDF6F28",
    "validated": false
  },
  "status": "success",
  "type": "response"
}

Note, I believe this is a different format than the case where the transaction is already in the open ledger and is likely to be validated by consensus soon. This type of response also omits several fields (ledger_index, meta, etc.) that are currently documented as always being present in the response. Related: XRPLF/clio#722.

This response does not provide enough information to know whether the transaction has succeeded or failed, because we don't know if server has all the ledgers from the min_ledger through the max_ledger specified in the request. (See "How to Reproduce" below for details of how to re-create and test this case.)

Solution

If the response in this case (tx found, not validated) included a searched_all field similar to in the txnNotFound response, then the client could use that information to conclusively determine that the transaction will never be validated in the case where validated is false and searched_all is true. (In the case where searched_all is false, the client has to either wait or ask another server depending on why some of the ledgers weren't searched.) This is a non-breaking API change that would vastly improve the usefulness of this response case.

Alternatively, I think it would be OK to remove this "cached but not in any ledger" case and return txnNotFound if the transaction is in the cache but not in any known ledger. I'm not sure if that counts as a breaking API change; in a sense it could be considered a bug fix to remove an anomalous case that doesn't match the documented response format.

It's not possible to add the date and ledger_index fields to this response type because those fields are inapplicable to transactions in this state. So, unless we remove this case and return txnNotFound, documentation changes are also necessary.

How to Reproduce

This case can be reproduced in stand-alone mode or on an actual network such as Testnet, in several ways. Two straightforward ways to ensure that a transaction is never validated are:

In both cases, the transaction is eventually evicted from cache and the result is txnNotFound instead, but shortly after submission (the time when you're most likely to want to know a transaction's result) the tx result is the ambiguous case in the example above.

Paths Not Taken

More Aggressive Eviction of Doomed Transactions

You may notice that in the above examples, rippled is caching and returning a transaction that can never succeed. Most likely this is because the server does not know that. There are similar cases where it might be possible for the transaction to eventually become validated (e.g. if you used a future sequence and then submitted the transactions that use the sequence numbers in between). Presumably the thought process behind the current behavior was that it's better to provide as much information about the transaction as is available, but the lack of searched_all means that finding the cached-but-not-applied transaction is now actually less useful than not finding it.

The server could be better about dropping a transaction from cache when that transaction can never succeed; the hard part there is setting the lower bound for ledgers that could contain the transaction; but there are a few ways you could do this:

While all this might be a useful optimization, cache management is hard, and it seems less comprehensive than just making the response more useful in the above case.

FirstLedgerSequence

A FirstLedgerSequence transaction field would put a hard lower bound on what ledger can include the transaction in the same way that LastLedgerSequence puts a hard upper bound. This has been proposed before but has not yet been implemented. I still think it's a good idea, but it would require an amendment and it doesn't fix the immediate problem with the anomalous tx response.

FirstLedgerSequence would be useful for determining the final result of a transaction, since the transaction instructions themselves become a self-contained definition of what the min_ledger/max_ledger bounds are. If a transaction has both fields, a server with the given range of ledgers on-hand could definitively say if such a transaction will never be validated. This could also help the server to decide sooner to evict these transactions from cache.

sublimator commented 1 year ago

@mDuo13

Yes!

sublimator commented 1 year ago

re: FirstLedgerSequence

intelliot commented 1 year ago

Potentially related: https://github.com/XRPLF/rippled/pull/3837 https://github.com/XRPLF/rippled/issues/2945