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

transactions that fail during consensus are ignored by Horizon #309

Open
MonsieurNicolas opened this Issue Feb 13, 2018 · 7 comments

Comments

Projects
None yet
7 participants
@MonsieurNicolas
Copy link

MonsieurNicolas commented Feb 13, 2018

See details stellar/stellar-core#1541

Net is:

  • a failed transaction is not recorded by Horizon (lookup by tx hash doesn't return anything)
  • the count of transactions reported as part of the ledgers endpoint is reporting the number of ingested transactions and the data attached to the transactions doesn't match what is recorded in core's history.

In the issue linked above, the ledger contained 3 transactions (not 2 as reported by the endpoint), and the tx indexes are wrong (as in, the first transaction reported is actually not the first transaction of the transaction set).

This may explain #305 as clients will timeout instead of getting notified of the failure.

@oryband

This comment has been minimized.

Copy link

oryband commented Feb 13, 2018

@MonsieurNicolas note that in #305 the transaction was added to the ledger 5 seconds after the transaction was submitted, but the HTTP POST request to Horizon still timed out after 60 seconds.

@oryband

This comment has been minimized.

Copy link

oryband commented Feb 14, 2018

@MonsieurNicolas see updates on #305, the problem was with stellar/quickstart docker image.

@salessandri

This comment has been minimized.

Copy link

salessandri commented Apr 1, 2018

This has been a pain to work with. I am trying to keep track of the state of the account based on the TX and operations that are associated with an account but keep finding inconsistencies due to this issue.

I opened a ticket in stellar/stellar-protocol#86. But the issue seems to be horizon. Copying here the bug description and expectations listed in that ticket.


Problem Description

It is impossible to keep track of the XLM balance of an account using its effects/operations/transactions.

In the presence of failed transactions, the TX fee is deducted from the source account but the TX is not recorded anywhere (or at least is not retrievable through Horizon).

This has the effect of modifying the account's balance. But the source of that effect cannot be tracked.

I came across this issue when trying to keep track the account balance only by subscribing to the account's TX feed.
If a TX from the account being tracked failed, I would get an inconsistency between the balance retrieved used the account endpoint and the one that was calculating using the TX feed.

Note: The same applies with TX sequence numbers a number is "skipped"

Relevance

I think this is a relevant issue because it boils down to accountability and audit.

There is no way to determine why 100 stroops dissappeared from the account balance.

Basic Example

The account GBPMLK3JT5WLEJSZ3PMO3HKHONEAVMJPGVCIELVIOYKRRYBAG5IS6QA7 on the testnet is an example of this.

This account was created using the friend bot, thus got a funding of XLM 10,000.
This was done in TX: a7a7b2e74d949644d286b43078f62671754cca66aa935b609476fa9b1740328d.

Then a TX was created to transfer XLM 50,000 to another account. This TX was signed and submitted, its hash: 81291e92d924e2dfb8117fccaa58dcb0dd0daecfa658cfb7a8931e94ee628b3f.
The TX failed as expected.

Now the account balance shows: XLM 9,999.9999900. The TX fee was deducted.

But when querying the TX for the account, the only TX listed is the one where the account creation happened.
No effects or operations apart from the account creation ones are listed.

@gutenye

This comment has been minimized.

Copy link

gutenye commented Jul 13, 2018

Is there any reason why Horizon does not store the failed transactions? It's a pain to work with when you need to build balance from history.

@bartekn

This comment has been minimized.

Copy link
Member

bartekn commented Jul 13, 2018

The code changes required to include failed transaction doesn't seem to be super complicated. We could also update the API in a non-breaking way, ex. Horizon will return failed transaction/operations only if the request includes include_failed=true GET param. @nullstyle am I missing something here? Seems like a lot of projects needs this for audits.

@nullstyle

This comment has been minimized.

Copy link
Member

nullstyle commented Jul 18, 2018

Horizon doesn't store failed transactions as an explicit design choice. The majority use case, in my estimation, was for clients that didn't need access to failed transactions beyond "Did this transaction I just posted to you fail?". This means that the majority case doesn't need to explicitly filter out irrelevant data and a much easier integration is achieved. I recognize this was a trade off.

@nullstyle am I missing something here

The only reason we don't have a include_failed=true feature is because we havent prioritized it yet; Also, I'm not particularly happy with the usability of that choice, but not such that I would veto a PR to add it. I'm not opposed to including it. There are significant implications on the query costs, but it's certainly doable to add this feature.

However, I advocate that we solve this problem with our forthcoming effort to make horizon more customizable with regards to how it ingests and presents data.

@bartekn

This comment has been minimized.

Copy link
Member

bartekn commented Sep 5, 2018

My thoughts regarding implementation of this feature.

Database

A new field success (?) is needed in history_transactions and history_operations tables. Additionally, we should add indexes that support "txs/ops per ledger", "txs/ops per account", "ops per tx" queries.

We should ensure that this does not affect performance.

Ingestion

Currently failed transactions are skipped. After removing this and adding a "success" status to the transaction we still need to ensure that failed transactions:

  • are not taken into account in asset_stats calculations,
  • do not create new records in history_accounts, history_assets, history_effects and history_trades tables.

We should probably allow trimming failed transactions/operations from a DB when someone is no longer interested in keeping them. And the other way: allow reingesting ledgers to add failed transactions.

Rest API

I think a new GET parameter include_failed (default: false to make this a non-breaking change) for /*/transactions and /*/operations endpoints if fine. Each transaction and operation object should have success field indicating whether it's a successful or failed tx (or part of tx).

This also applies to streaming endpoints affected by the change.

Other things we should discuss:

  • Should /effects for a failed transaction return an empty response ("records": []) or 404 error to distinguish itself from tx with no effects?
  • Is result_xdr field for failed transactions enough or should we decode it to give more information about the reason of the failure?

Config

This feature is not needed by everyone but require much more disk space. So I agree with @nullstyle that this should be configurable. The ingestion system will be definitely extended with feature flags in a future to allow partial ingestion but for now a simple boolean setting should be fine.

SDKs

All SDKs must be updated with new request params and response fields.

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