TransactionLogger.waitForIndexer waits for failed transactions and transactions that have already been found

[joe-p]

Subject of the issue

There are currently two issues with waitForIndexer

1. waitForIndexer does not track which transactions have already been found, meaning it will waste time waiting for a transaction that is already known to be in indexer
2. waitForIndexer will wait (unscessfully) for failed transactions to show up indexer

There are two potential solutions

1. Use round instead of transaction ids in waitForIndexer (part of fix: waitForIndexer waiting for transactions that failed #358)
2. Pop transactions that have been found or failed (likely a separate array)

[robdmoore]

Using latest round defeats the purpose of being scoped to the transactions in the logger, but what could be done is look up the transaction id(s) in the logger (the latest one is probably enough unless it's an invalid transaction?) and then wait for the round of that particular transaction.

[joe-p]

Do we expect someone to use the transaction logger to send some transactions and then not use for others? I suppose it's just not clear to me why/how this would be used externally. Using transactions adds a lot of complexity vs simply asking "is indexer caught up" because we'd also need to have logic that handles failed transactions

[robdmoore]

We can use "indexer caught up to algod" for sure, it's just slower if you are doing parallel test runs.

Do we expect someone to use the transaction logger to send some transactions and then not use for others?

Not sure I understand this sentence? The logger isn't sending transactions just logging when they are sent.

[joe-p]

Ah didn't think of parallel tests... I meant do we expect the logger to not have the latest transactions that were sent to algod, and in the case of parallel tests (where each test has its own instance of logger) then the answer could be yes.

How much slower are we talking? Is the slowdown intermittent or is indexer consistently slow in catching up? I typically avoid using indexer so it's been awhile since I've had first hand experience with it. I'm just trying to gauge whether it's worth adding the complexity for tracking failed transactions

[robdmoore]

The other option is do nothing. It's so unlikely with per test scoping that it's even an issue if we are only waiting for the last transaction.

[robdmoore]

(this behaviour has been there since this functionality was first implemented and noone has complained about it except when it was waiting for all transactions with global scoping, which will naturally show the problem up)

[joe-p]

I don't think ignoring a bug in something that is publicly exposed is a reasonable path forward. I would imagine it's never caused known issues before because we actively discourage people from using indexer at all so most users don't even have a need for waitForIndexer, let alone know about its existence. The reality is that we have a public function that has a known issue. Just because its not often used doesn't mean it should be ignored which is why I want to explore the solution space.

[robdmoore]

I agree in principle, but my point is just this is an edge case since most times you call this will be right after you have issued transactions you've waited for (and thus they will throw before you hit this method).

[robdmoore]

Either way, no issue in it being solved, but I think when the other change is merged that fixes the implementation to only check the latest txid the urgency is low.

[robdmoore]

side-note: having a method that waits for indexer to catch up to latest algod round is a perfectly valid method to have, and we may even decide it makes sense to expose as the main waitForIndexer method from the test context, but for clarity that implementation doesn't make sense as the transactionLogger.waitForIndexer method since it would have nothing to do with the transaction logger anymore.

[joe-p]

I agree in principle, but my point is just this is an edge case since most times you call this will be right after you have issued transactions you've waited for (and thus they will throw before you hit this method).

My main concern is tests that involve catching an exception from a failed transaction. It would be one thing if it threw an error, but indefinitely hanging is a really nasty behavior to debug. I agree the chance of this occurring is low, especially considering the usage of the function itself is low, but I don't love having something that can break in an ugly way.

If we don't want to do "wait for last round" (which would be a breaking change, but #355 is going to be a breaking change anyways) I would rather have logic that pops reject txns than doing nothing despite the additional complexity.

[joe-p]

Thinking about this recently. I think the best solution is we add an two optional parameters (maybe single object?)

type WaitUntil = 
  | { waitUntil: 'allTxnsConfirmed' | 'lastValidRound' ; waitUntilRound?: never }
  | { waitUntil?: never; waitUntilRound: number }

allTxnsConfirmed: Current behavior. This is the default

lastValidRound waits until the latest lastVaid across all transactions is reached

waitUntilRound waits until a specific round is reached.

For those that run into edge cases this provides a solution without a breaking change