services/horizon: Avoid combining SQL clauses when filtering is disabled

[Shaptic]

What

Stop adding the UNION ALL clause alongside a filter for transaction hashes in the history_transactions_filtered_tmp table when ingestion filtering is disabled.

Previously, the final SQL statement would look something like this:

ListOfPendingTxs = [ ... ]

SELECT `...[fields from ht and hl]...`
    FROM
        history_transactions ht LEFT JOIN history_ledgers hl 
        ON ht.ledger_sequence = hl.sequence 
    WHERE 
        (ht.ledger_sequence >= $1 AND 
        (ht.transaction_hash IN ListOfPendingTxs) OR 
        (ht.inner_transaction_hash ListOfPendingTxs))
UNION ALL 
SELECT `...[fields from ht and hl]...`
    FROM
        history_transactions_filtered_tmp ht LEFT JOIN history_ledgers hl 
        ON ht.ledger_sequence = hl.sequence 
    WHERE 
        (ht.ledger_sequence >= $1 AND 
        (ht.transaction_hash IN ListOfPendingTxs) OR 
        (ht.inner_transaction_hash ListOfPendingTxs))

whereas now it will only include the portion prior to the UNION ALL clause.

Why

There's no reason to have the additional clause when the history_transactions_filtered_tmp table is guaranteed to be empty if the EXP_ENABLE_INGESTION_FILTERING flag is set to false.

Known limitations

n/a

[tamirms]

I think it would be easier to read if we had a FilteringEnabled field. That way you could pass in the field directly to AllTransactionsByHashesSinceLedger() without inverting it

[sreuland]

the ingestion filtering enabled config flag is true by default, it's marked deprecated, won't be override-able soon. for most deployments, it's probably not being set, and defaulting to true.

may need to continue to check the config flag for now but also check that at least one filter rule is enabled to determine if effectively disabled, can probably wrap that as new method on the QFilter interface:

func (q *Q) IsFilteringEffective(ctx context.Context) (bool, error) {
	accountFilter, err := q.GetAccountFilterConfig(ctx)
	if err != nil {
		return false, err
	}

	assetFilter, err := q.GetAssetFilterConfig(ctx)
	if err != nil {
		return false, err
	}

	return accountFilter.Enabled || assetFilter.Enabled, nil
}

in db migrations, both rules were seeded in the db as disabled by default.

[tamirms]

if we're going to deprecate the flag and always enable filtering then I think we should leave the pending transactions query as is because, the over head of having to query the db on whether filtering rules are present is probably more inefficient than getting rid of the union in the pending transactions query. There is also the problem that querying the db on whether filtering rules are present is still vulnerable to a race condition where filtering rules are added right after we query the db.

[sreuland]

yes, agree on that summary, 👍

[Shaptic]

ah wow, I didn't realize we were deprecating the flag entirely! in that case there's no benefit to having this piece of code, since as you said @tamirms we'd need to actively query for filters to avoid a(n empty) subquery which defeats the purpose. thanks for the discussion!

[tamirms]

could you add a test which exercises both code paths where includeFiltered is true / false?

[Shaptic]

counterproductive per the above discussion: #5250 (comment)