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 command fails to return transactions that can't be submitted #2597

Open
mDuo13 opened this issue Jun 22, 2018 · 4 comments
Open

tx command fails to return transactions that can't be submitted #2597

mDuo13 opened this issue Jun 22, 2018 · 4 comments

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented Jun 22, 2018

(Original issue: RIPD-1098)

The tx method cannot fetch pseudo-transactions, nor others transactions that aren't currently considered "valid" for submission, even if those transactions were incorporated into a validated ledger under rules that applied at the time. Doing so returns a misleading "not found" error. Example of a tx websocket command that will always fail (use a full-history server to confirm):

{
  "command": "tx",
  "transaction": "5B1F1E8E791A9C243DD728680F108FEF1F28F21BA3B202B8F66E7833CA71D3C3"
}

The transaction_entry method can fetch such transactions, provided you know what ledger contains them. Example of a transaction_entry websocket command that succeeds at fetching the same:

{
  "id": 2,
  "command": "transaction_entry",
  "tx_hash": "5B1F1E8E791A9C243DD728680F108FEF1F28F21BA3B202B8F66E7833CA71D3C3",
  "ledger_index": 21225473
}

Per @JoelKatz:

This occurs because the "tx" command asks the TransactionMaster to "fetch" the transaction, which calls Transaction::load. This function calls "checkValidity" to assure the returned transaction is valid. But this makes no sense. You should be able to retrieve transactions that are illegal under current rules. And pseudo-transactions are always illegal to submit, which is what checkValidity checks.

For historical retrieval, the only check should be that we can deserialize the binary transaction data, and we should return an error other than "not found" in that case (and we can include the ledger sequence in the RPC, which would be helpful).

So, the changes to make would be:

  • Remove the validity check from the tx command. Return it if we can deserialize it.
  • If we can't deserialize the transaction, return a different error. I suggest a new error code, txnDeserializingError. Also, this should be logged at the warning level, because rippled should be able to deserialize any transaction in valid history (even if that transaction is no longer valid under current rules).
@mDuo13 mDuo13 added the Bug label Aug 1, 2019
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Sep 25, 2019
Historical Transaction retrieval no longer needs to pass current validity check;
Introduced additional RPC error code for database deserialization error.
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Sep 26, 2019
Historical Transaction retrieval no longer needs to pass current validity check;
Introduced additional RPC error code for database deserialization error.
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Oct 28, 2019
… pass current validity check; introduced additional RPC error code for database deserialization error.
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Oct 28, 2019
… pass current validity check; introduced additional RPC error code for database deserialization error.
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Oct 29, 2019
* historical tx retrieval no longer needs to pass current validity check;
* introduced additional RPC error code for database deserialization error.
ShangyanLi added a commit to ShangyanLi/rippled that referenced this issue Nov 1, 2019
* historical tx retrieval no longer needs to pass current validity check;
* introduced additional RPC error code for database deserialization error.
undertome pushed a commit to undertome/rippled that referenced this issue Nov 11, 2019
* historical tx retrieval no longer needs to pass current validity check;
* introduced additional RPC error code for database deserialization error.
@cjcobb23
Copy link
Contributor

@mDuo13 This has been resolved here: 11cf27e
Can you close this issue?

@mDuo13 mDuo13 added this to the v1.5.0 milestone Feb 4, 2020
@mDuo13
Copy link
Collaborator Author

mDuo13 commented Feb 4, 2020

I'll test this out on a 1.5.0 beta server soon and close it when I confirm it works as expected.

@carlhua carlhua removed this from the v1.5.0 milestone Mar 13, 2020
@carlhua
Copy link
Contributor

carlhua commented Mar 30, 2020

Closing this issue - this issue is fixed in 11cf27e and should have been closed by #3167

@mDuo13
Copy link
Collaborator Author

mDuo13 commented May 11, 2023

This problem seems to have come back—but it only applies to Reporting Mode. (To repro: take the example requests from the OP and test them on s1/s2 vs xrplcluster.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants